Creating a new app in the current directory #368

torifat · 2016-08-05T01:24:22Z

This fixes #334

torifat · 2016-08-05T03:12:36Z

@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

vjeux · 2016-08-05T03:51:35Z

scripts/init.js

-  // 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'));


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 · 2016-08-05T10:02:17Z

@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.

gaearon · 2016-08-05T11:54:16Z

path-exists currently installed as dependencies of other modules

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

gaearon · 2016-08-05T11:56:39Z

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?

torifat · 2016-08-05T13:44:05Z

@gaearon currently in case of existing .gitignore I'm showing this:

Sorry, I'm not good with words 😞

vjeux · 2016-08-05T14:12:24Z

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 · 2016-08-05T14:14:38Z

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 · 2016-08-05T14:15:08Z

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

vjeux · 2016-08-05T15:07:12Z

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 · 2016-08-05T15:12:07Z

Files we will allow for now:

.git
.gitignore
README.md
LICENSE

torifat · 2016-08-05T15:20:50Z

Virgin repo in github:

vjeux · 2016-08-05T15:25:10Z

👍

torifat · 2016-08-06T00:41:16Z

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

torifat · 2016-08-06T00:53:50Z

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 · 2016-08-06T01:32:30Z

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

Jiansen · 2016-08-06T10:47:44Z

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

my-app should not exists in the current directory. my-app will be used as app name (same as current)
rungit clone (@torifat what is the default app name when running create-react-app ./?)
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
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

run git init when initial an app,
after the app is generated, print out the instruction of adding existing project to github.

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

vjeux · 2016-08-06T16:39:51Z

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 · 2016-08-06T20:28:26Z

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.

torifat · 2016-08-06T21:35:42Z

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

Bail if existing directory contains file other than any of the following:
- .DS_Store
- Thumbs.db
- .git
- .gitignore
- README.md
- LICENSE
Rename README.md to README.old.md
Append our .gitignore if there's any existing one.

Jiansen · 2016-08-06T22:46:31Z

@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.

vjeux · 2016-08-07T21:34:05Z

scripts/init.js

+    if (err) {
+      // Append if there's already a `.gitignore` file there
+      if (err.code === 'EEXIST') {
+        fs.readFile(path.join(appPath, 'gitignore'), (err, data) => {


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

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

vjeux · 2016-08-07T21:40:16Z

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

torifat · 2016-08-07T23:05:28Z

@vjeux fixed.

vjeux · 2016-08-08T04:06:12Z

Yay, thanks!

denofevil · 2016-08-30T13:54:40Z

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:

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?
react-create-app package with current dir generation is not published yet, but obviously it would be better to solve 1 first

geowarin · 2016-08-30T14:45:46Z

Why not ignore all dot files?

ForbesLindesay · 2016-08-30T14:51:48Z

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

gaearon · 2016-08-30T20:17:35Z

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 · 2016-08-31T11:59:40Z

@gaearon, thank you, added #522

* Creating a new app in the current directory * Fixed style mistakes

tofagerl · 2016-12-23T11:08:41Z

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

denofevil · 2016-12-23T11:10:06Z

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

tofagerl · 2016-12-23T23:31:27Z

Great! :)

Velua · 2017-03-20T16:23:36Z

global-cli/index.js

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


Can we add 'CNAME' to the the validFiles array? https://help.github.com/articles/adding-or-removing-a-custom-domain-for-your-github-pages-site/

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

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.

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

ghost added the CLA Signed label Aug 5, 2016

vjeux reviewed Aug 5, 2016
View reviewed changes

ghost added the CLA Signed label Aug 5, 2016

ghost added the CLA Signed label Aug 6, 2016

Jiansen mentioned this pull request Aug 6, 2016

Auto-set a generated app as a git repo #385

Closed

ghost added the CLA Signed label Aug 6, 2016

Creating a new app in the current directory

a52fa2e

ghost added the CLA Signed label Aug 6, 2016

vjeux reviewed Aug 7, 2016
View reviewed changes

Fixed style mistakes

fe12b8d

facebook-github-bot added the CLA Signed label Aug 7, 2016

vjeux merged commit e26b8d9 into facebook:master Aug 8, 2016

torifat deleted the feature/init-in-existing-dir branch August 8, 2016 14:47

gaearon mentioned this pull request Aug 31, 2016

ignore .idea folder #522

Merged

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 (facebook#368)

7fbbdd5

* 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 (facebook#368)

46fe4a6

* Creating a new app in the current directory * Fixed style mistakes

driquelme mentioned this pull request Feb 21, 2017

Fix conflict file issue in IJ static web projects by adding web.iml to the exclusion list #1603

Merged

Velua reviewed Mar 20, 2017

View reviewed changes

lock bot locked and limited conversation to collaborators Jan 21, 2019

Creating a new app in the current directory #368

Creating a new app in the current directory #368

Conversation

torifat commented Aug 5, 2016 • edited Loading

torifat commented Aug 5, 2016

vjeux Aug 5, 2016

Choose a reason for hiding this comment

Jiansen commented Aug 5, 2016

gaearon commented Aug 5, 2016

gaearon commented Aug 5, 2016

torifat commented Aug 5, 2016 • edited Loading

vjeux commented Aug 5, 2016

gaearon commented Aug 5, 2016

gaearon commented Aug 5, 2016

vjeux commented Aug 5, 2016

torifat commented Aug 5, 2016 • edited Loading

torifat commented Aug 5, 2016

vjeux commented Aug 5, 2016

torifat commented Aug 6, 2016

torifat commented Aug 6, 2016

torifat commented Aug 6, 2016

Jiansen commented Aug 6, 2016

another thought

vjeux commented Aug 6, 2016

Jiansen commented Aug 6, 2016

torifat commented Aug 6, 2016 • edited Loading

Jiansen commented Aug 6, 2016

vjeux Aug 7, 2016

Choose a reason for hiding this comment

torifat Aug 7, 2016

Choose a reason for hiding this comment

vjeux commented Aug 7, 2016

torifat commented Aug 7, 2016

vjeux commented Aug 8, 2016

denofevil commented Aug 30, 2016

geowarin commented Aug 30, 2016

ForbesLindesay commented Aug 30, 2016

gaearon commented Aug 30, 2016

denofevil commented Aug 31, 2016

tofagerl commented Dec 23, 2016

denofevil commented Dec 23, 2016

tofagerl commented Dec 23, 2016

Velua Mar 20, 2017

Choose a reason for hiding this comment

Timer Mar 20, 2017

Choose a reason for hiding this comment

pstanton Jun 22, 2017

Choose a reason for hiding this comment

gaearon Jun 22, 2017

Choose a reason for hiding this comment

torifat commented Aug 5, 2016 •

edited

Loading

torifat commented Aug 5, 2016 •

edited

Loading

torifat commented Aug 5, 2016 •

edited

Loading

torifat commented Aug 6, 2016 •

edited

Loading