Skip to content
New issue

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

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

Already on GitHub? # to your account

Add ES5 version of path-exists to CLI #617

Closed
wants to merge 1 commit into from
Closed

Add ES5 version of path-exists to CLI #617

wants to merge 1 commit into from

Conversation

sotojuan
Copy link

See #570 (comment), though it may not be needed as the issue was closed.

I couldn't find a way to put it in its own file and have both global-cli/index.js and the stuff under scripts/ use it (I kept getting errors that the file was not found in the CLI) so I just put in the file that needed it :p

@ghost ghost added the CLA Signed label Sep 10, 2016
@fson
Copy link
Contributor

fson commented Sep 10, 2016

Thanks @sotojuan.

The readme of path-exists warns against checking if a path exists before doing IO:

In particular, checking if a file exists before opening it is an anti-pattern that leaves you vulnerable to race conditions: another process may remove the file between the calls to fs.exists() and fs.open(). Just open the file and handle the error when it's not there

To avoid potential race conditions, maybe it would be better, if we would simply try to create the dir and just handled the EEXIST error? This is how the mkdirp package works, so perhaps we can use it. Basically:

var createdPath = mkdirp(root);
if (!createdPath && !isSafeToCreateProjectIn(root)) {
  console.log('The directory `' + name + '` contains file(s) that could conflict. Aborting.');
  process.exit(1);
}

Please also include a test plan in the pull request.

@sotojuan
Copy link
Author

Can the race condition happen even if we use the sync version?

@fson
Copy link
Contributor

fson commented Sep 13, 2016

@sotojuan I'd say it's a pretty unlikely scenario in this case, but what could happen in theory is that in the time between checking if a directory exists and creating it, some other process could create it, which would result in an error (we would try to create a directory that already exists). It would happen regardless of if we use the sync or async version.

mkdirp handles this by doing both steps at once (it just tries to create it and handles the error if it already exists). It also handles creating longer paths like ./some/nested/directory/my-app, which we don't seem to currently handle correctly. My thinking is simply, if we're changing this code, we might as well do it right and use mkdirp?

@ghost ghost added the CLA Signed label Sep 13, 2016
@sotojuan
Copy link
Author

Got it. I have a busy week but I'll definitely look into this—looks like an interesting problem to solve :-)

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

Since we already had this issue, seems like there is no need to block this PR, it being a strict improvement.

@gaearon gaearon added this to the 0.4.2 milestone Sep 16, 2016
@sotojuan
Copy link
Author

In that case, I'm fixing merge conflicts and will push after making sure tests pass.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

Merged via fc3ab46, thanks!

@gaearon gaearon closed this Sep 16, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

Haha, didn’t expect you’d answer so quickly. No worries, it’s in master now 😄

@SimenB
Copy link
Contributor

SimenB commented Sep 19, 2016

Why not just version 2 of the module? Has to be better than using a deprecated function, right?

@sotojuan sotojuan deleted the es5-path-exists branch September 19, 2016 21:14
@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2016

Feel free to submit a PR. I didn’t know there’s an earlier version.

@SimenB
Copy link
Contributor

SimenB commented Sep 19, 2016

Done, see #685 😄

fson pushed a commit that referenced this pull request Sep 19, 2016
* Revert "Add ES5 version of `path-exists` to CLI"

This reverts commit fc3ab46.

* Use pre node@4 compatible `path-exists`

Ref #617
feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Revert "Add ES5 version of `path-exists` to CLI"

This reverts commit fc3ab46.

* Use pre node@4 compatible `path-exists`

Ref facebook#617
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants