Skip to content

Conversation

@fivetanley
Copy link
Contributor

fixes #28
i may have also extracted that trimLeft functionality

@rkostrzewski
Copy link
Collaborator

I would add several more patterns to gitignore:

  • ignoring logs:
*.log
npm-debug.log*
yarn-debug.log*
yarn-error.log*
  • ignoring coverage (preact-cli will be using jest AFAIK which uses istanbul)
coverage

The default gitignore for node might have some more goodies but frankly I haven't seen half of those files in any node project.

@lukeed
Copy link
Member

lukeed commented May 22, 2017

^^ With *.log in place, no need for these:

npm-debug.log*
yarn-debug.log*
yarn-error.log*

@rkostrzewski
Copy link
Collaborator

I remember npm generating timestamps in log file names but it doesn't do so anymore (at least I don't remember how it is done) so probably they are not needed. :)

// Initializes the folder using `git init` and a proper `.gitignore` file
// if `git` is present in the $PATH.
const initializeVersionControl = async function(target) {
if (which.sync('git')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which.sync throws an error if command is not found according to the docs. Is the exception swallowed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using the async version of which. Here's how:

import promisify from 'es6-promisify';

// ...

async function initializeVersionControl(target) {
  let git;
  try {
    git = await promisify(which)('git');
  }
  catch (err) {}
  if (!git) {
    // ... etc
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this have to be async? It would throw in either case when using async/await.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to avoid blocking (nothing else is blocking in the CLI).

await spawn('git', ['init'], { cwd });
await spawn('git', ['add', '-A'], { cwd });

const gitUser = 'Preact CLI<[email protected]>';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heh - I am wondering if this will spam me with notifications 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of dear !! That sure will. Good luck with that !!

@developit
Copy link
Member

Overall this is great! Thanks for doing all the work, haha.

Only thing I'm keen to add to this aside from the little sync fix would be a git option that defaults to true, so someone can do preact create --no-git to avoid initializing a repo if they wish. I'm happy to add that after merging if it's not of interest :)

Copy link
Member

@lukeed lukeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I would like to see a which abstraction that handles its own error case -- especially now that we have conditional Yarn usage.

I can add this in post-merge if needed.

@developit
Copy link
Member

@lukeed agreed, it would be nice to have commandExists('yarn') that just returns a Boolean without the try/catch.

@rkostrzewski
Copy link
Collaborator

@developit
Copy link
Member

developit commented May 29, 2017

wow I even subconsciously stole the name.

One thought I had regarding the original topic here - what do we think of actually using git to clone starter projects? I believe vue-cli does this. There's just a github org for the templates, and init just clones them and does the installation. I normally try to avoid requiring git (not sure why), but the huge advantage would be that new templates can be released independent of the CLI. Also, anyone could then run preact create git://foo and start from their own template.

Thoughts? I think they were using a module that handled like 90% of the work too.

@developit
Copy link
Member

I think this makes more sense to be paired up with #56 as a minor or even major.

@lwakefield lwakefield mentioned this pull request Jun 10, 2017
@developit developit merged commit a7c19fc into preactjs:master Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No git repo initiated with project

5 participants