Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Creating a new app in the current directory #368

Merged
merged 2 commits into from
Aug 8, 2016

Conversation

torifat
Copy link
Contributor

@torifat torifat commented Aug 5, 2016

This fixes #334

@ghost ghost added the CLA Signed label Aug 5, 2016
@torifat
Copy link
Contributor Author

torifat commented Aug 5, 2016

@gaearon how to fix this test?

/tmp/tmp.PR0pxHlG7e/node_modules/create-react-app/index.js --scripts-version=/home/travis/build/facebookincubator/create-react-app/react-scripts-0.3.0-alpha.tgz test-app

@ghost ghost added the CLA Signed label Aug 5, 2016
// Rename gitignore after the fact to prevent npm from renaming it to .npmignore
// See: https://github.com/npm/npm/issues/1862
fs.move(path.join(appPath, 'gitignore'), path.join(appPath, '.gitignore'), []);
var gitignoreExists = pathExists.sync(path.join(appPath, '.gitignore'));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is why I originally wanted to only support the non existing folder case. Now we get in this situation where we have some files that already exist and need to be somehow merged. We can no longer be sure that all the configs that we have all work well together.

Could we check all the files that would be copied over and if any of them is already there then abort.

Honestly, i'm not sure it's worth the complexity, but I could be convinced otherwise

@Jiansen
Copy link
Contributor

Jiansen commented Aug 5, 2016

@torifat

@gaearon how to fix this test?

/tmp/tmp.PR0pxHlG7e/node_modules/create-react-app/index.js --scripts-version=/home/travis/build/facebookincubator/create-react-app/react-scripts-0.3.0-alpha.tgz test-app

Following is the context I saw in the latest CI console (build 423.2)

++++mktemp -d
+++temp_app_path=/tmp/tmp.qXZBl8NKp9
+++cd /tmp/tmp.qXZBl8NKp9
+++node /tmp/tmp.bMGafb5xCy/node_modules/create-react-app/index.js --scripts-version=/home/travis/build/facebookincubator/create-react-app/react-scripts-0.3.0-alpha.tgz test-app
module.js:442
    throw err;
    ^
Error: Cannot find module 'path-exists'
...

Checking in your init-in-existing-dir branch, I don't see changes to the package.json file neither.
path-exists currently installed as dependencies of other modules. Because you use var pathExists = require('path-exists'); in your changes, I guess that you need to run npm install --save path-exists and commit your changes in package.json as well.

@ghost ghost added the CLA Signed label Aug 5, 2016
@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

path-exists currently installed as dependencies of other modules

Yep, this would only work with npm 3, but not with npm 2.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

I think we should do the following:

  • Allow . if the directory has zero conflicts with copied files
  • Allow . if the directory has .gitignore (we won’t copy our own then)
  • Abort in any other case

@vjeux Thoughts?

@ghost ghost added the CLA Signed label Aug 5, 2016
@torifat
Copy link
Contributor Author

torifat commented Aug 5, 2016

@gaearon currently in case of existing .gitignore I'm showing this:
screenshot 2016-08-05 19 42 05

Sorry, I'm not good with words 😞

@ghost ghost added the CLA Signed label Aug 5, 2016
@vjeux
Copy link
Contributor

vjeux commented Aug 5, 2016

For the no conflicts, we will only know after spending a minute doing npm install. Do you think it's going to be a good experience? We could check for package.json and node_modules folder early though which may solve a lot of cases.

If you init the project via github there's also a change that it creates a dummy README file that may also conflict with our own.

For gitignore, what is the scenario when one already exists? When doing git init does it create one? If yes, I'm pretty sure that we do want to override it with ours as it contains a bunch of useful things.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

Let's be more strict and only allow empty directories or directories with .git then.

I think whatever GitHub creates when you make a new project should be allowed.

@gaearon
Copy link
Contributor

gaearon commented Aug 5, 2016

I totally agree we should only stick with checks we can do immediately.

@vjeux
Copy link
Contributor

vjeux commented Aug 5, 2016

Okay I chatted with Dan offline and here's the proposal we're happy with:

  • Look at all the files in that folder and if there's any file that is not one that's automatically generated by github when generating a new project:.git, .gitignore, README and LICENSE (need to double check there may be more), then bail immediately.
  • If there is README prompt that we're going to erase it, do you want to continue?
  • proceed with the installation and when it is time for gitignore, concat the two files instead of skipping ours as it is right now.

Thanks for working on this, it's going to be super useful!

@torifat
Copy link
Contributor Author

torifat commented Aug 5, 2016

Files we will allow for now:

  • .git
  • .gitignore
  • README.md
  • LICENSE

@torifat
Copy link
Contributor Author

torifat commented Aug 5, 2016

Virgin repo in github:
screenshot 2016-08-05 21 20 09

@vjeux
Copy link
Contributor

vjeux commented Aug 5, 2016

👍

@torifat
Copy link
Contributor Author

torifat commented Aug 6, 2016

@vjeux what about .DS_Store in macOS and Thumbs.db in Windows?

@ghost ghost added the CLA Signed label Aug 6, 2016
@torifat
Copy link
Contributor Author

torifat commented Aug 6, 2016

Also can you guys help me with a better error message. Currently, I'm showing The directorycontains file(s) that could conflict. Aborting. But, it feels lame 😞

@torifat
Copy link
Contributor Author

torifat commented Aug 6, 2016

@gaearon we have a prompt in utils/. And, now need prompt in global-cli. What do you suggest?

@ghost ghost added the CLA Signed label Aug 6, 2016
@Jiansen
Copy link
Contributor

Jiansen commented Aug 6, 2016

I usually initial a git repo via web interface before working on a local dev folder. If this is the only use case where we want to initialise an app in an existing folder, how about use the following command?

$ create-react-app my-app --git https://github.com/username/my-app.git
  1. my-app should not exists in the current directory. my-app will be used as app name (same as current)
  2. rungit clone (@torifat what is the default app name when running create-react-app ./?)
  3. check if the cloned git repo contain files other than .gitignore .git README.md etc.
    • @torifat has done the work
    • conflict_check could be a function which we could update the policy in later versions
  4. initialise my-app if there is no potential conflict

With this workflow, we may not need to worry about the following 2 problems

@vjeux what about .DS_Store in macOS and Thumbs.db in Windows?
@gaearon we have a prompt in utils/. And, now need prompt in global-cli. What do you suggest?

We don't need a prompt in global-cli because we don't need to ask user's opinion when working on an almost empty folder. As for the original README.md, we could let user know that the tool renamed it as README.old.md. Users will appreciate our detailed instructions in README. It is not much work to merge the initial short README to the new one.

another thought

one alternative is adding the following 2 steps to the current init process

We then even saved users a few minutes of visiting github webpage.

@ghost ghost added the CLA Signed label Aug 6, 2016
@vjeux
Copy link
Contributor

vjeux commented Aug 6, 2016

For DSStore and the windows one, good call, we should add them to the list of things that are okay of a fresh install.

I like the suggestion of renaming README.md into README.old.md, it lets us avoid adding a prompt, let's do that.

For a --git command or automatically calling git init, we should consider it but it is out of scope for this pull request, could you open an issue about it so we can discuss it properly? Thanks!

@Jiansen
Copy link
Contributor

Jiansen commented Aug 6, 2016

For a --git command or automatically calling git init, we should consider it but it is out of scope for this pull request, could you open an issue about it so we can discuss it properly? Thanks!

From the discussion so far, especially from the following rules @vjeux and @gaearon set. I am very convinced that CRA should be able to generated app in a freshly created git repo. In deed, I couldn't agree more that code should be managed using git. I am happy to make the process of setting git repo as easy as possible for users.

  • Look at all the files in that folder and if there's any file that is not one that's automatically generated by github when generating a new project:.git, .gitignore, README and LICENSE (need to double check there may be more), then bail immediately.
  • If there is README prompt that we're going to erase it, do you want to continue?
  • proceed with the installation and when it is time for gitignore, concat the two files instead of skipping ours as it is right now.

Apart from generating app in a freshly created git repo, I don't come up with other practical use cases that users want to generate app in an existing directory, but @torifat may have good ideas. @torifat , if you had some use cases in your mind, could you share with us because some of those use cases may be useful but not fit the rules above.

@ghost ghost added the CLA Signed label Aug 6, 2016
@torifat
Copy link
Contributor Author

torifat commented Aug 6, 2016

@Jiansen, please check @vjeux's last comment. I have updated the pull request. It does the following:

  1. Bail if existing directory contains file other than any of the following:
    • .DS_Store
    • Thumbs.db
    • .git
    • .gitignore
    • README.md
    • LICENSE
  2. Rename README.md to README.old.md
    screenshot 2016-08-07 03 33 25
  3. Append our .gitignore if there's any existing one.

@ghost ghost added the CLA Signed label Aug 6, 2016
@Jiansen
Copy link
Contributor

Jiansen commented Aug 6, 2016

@torifat ,

Good job and many thanks for the work. With your work, I don't really need a --git tag. What I am wondering is how users use CRA in practice. If managing code using git is the only use case that a user wants to create an app in an existing directory, we may be able to make the process a bit more convenient on top of your work.

@ghost ghost added the CLA Signed label Aug 6, 2016
if (err) {
// Append if there's already a `.gitignore` file there
if (err.code === 'EEXIST') {
fs.readFile(path.join(appPath, 'gitignore'), (err, data) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use readFileSync, otherwise we may run into race conditions as the rest of the file is going to run concurrently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we do nothing with that file later, so I thought it would be ok. Changing it.

@vjeux
Copy link
Contributor

vjeux commented Aug 7, 2016

Looks really good! Let's fix the small nits and i'll merge it ;)

@torifat
Copy link
Contributor Author

torifat commented Aug 7, 2016

@vjeux fixed.

@vjeux vjeux merged commit e26b8d9 into facebook:master Aug 8, 2016
@vjeux
Copy link
Contributor

vjeux commented Aug 8, 2016

Yay, thanks!

@torifat torifat deleted the feature/init-in-existing-dir branch August 8, 2016 14:47
@denofevil
Copy link
Contributor

Hi,
Dennis from the WebStorm team here. We'd like to reuse this part for react project generation, however there are two things that stop us:

  1. IntelliJ project information files are generated before generator is actually invoked, so we have .idea folder that is considered conflicting. Is that ok to create PR adding that file to the list?
  2. react-create-app package with current dir generation is not published yet, but obviously it would be better to solve 1 first

@geowarin
Copy link
Contributor

Why not ignore all dot files?

@ForbesLindesay
Copy link
Contributor

Travis is configured via a dot file, as are many other systems, so we can't just blanket ignore all dot files.

@gaearon
Copy link
Contributor

gaearon commented Aug 30, 2016

  1. IntelliJ project information files are generated before generator is actually invoked, so we have .idea folder that is considered conflicting. Is that ok to create PR adding that file to the list?

Sounds good to me, let’s do it. 👍

@denofevil
Copy link
Contributor

@gaearon, thank you, added #522

@gaearon gaearon mentioned this pull request Aug 31, 2016
stayradiated pushed a commit to stayradiated/create-react-app that referenced this pull request Sep 7, 2016
* Creating a new app in the current directory

* Fixed style mistakes
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Creating a new app in the current directory

* Fixed style mistakes
@tofagerl
Copy link

This still errors out when you use an .iml file instead of a .idea folder.

@denofevil
Copy link
Contributor

@tofagerl latest IDEA (2016.3.2RC) should work ok with that

@tofagerl
Copy link

Great! :)


function isGitHubBoilerplate(root) {
var validFiles = [
'.DS_Store', 'Thumbs.db', '.git', '.gitignore', 'README.md', 'LICENSE'
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you open a new issue with this request please? It's a little difficult to track in an issue like this.

Choose a reason for hiding this comment

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

this essentially makes it impossible to use another collaboration/versioning product eg subversion, or some other IDE such as eclipse to register the project. I am trying to co-locate my project in eclipse and use subversion for collaboration. Eclipse creates a ".project" file, and subversion creates ".svn" directories.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please open another issue to discuss this. Comments on old issues aren't very actionable.

@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a new app in the current directory