-
-
Notifications
You must be signed in to change notification settings - Fork 27k
Add ES5 version of path-exists
to CLI
#617
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
Conversation
Thanks @sotojuan. The readme of
To avoid potential race conditions, maybe it would be better, if we would simply try to create the dir and just handled the 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. |
Can the race condition happen even if we use the |
@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.
|
Got it. I have a busy week but I'll definitely look into this—looks like an interesting problem to solve :-) |
Since we already had this issue, seems like there is no need to block this PR, it being a strict improvement. |
In that case, I'm fixing merge conflicts and will push after making sure tests pass. |
Merged via fc3ab46, thanks! |
Haha, didn’t expect you’d answer so quickly. No worries, it’s in master now 😄 |
Why not just version 2 of the module? Has to be better than using a deprecated function, right? |
Feel free to submit a PR. I didn’t know there’s an earlier version. |
Done, see #685 😄 |
* Revert "Add ES5 version of `path-exists` to CLI" This reverts commit fc3ab46. * Use pre node@4 compatible `path-exists` Ref facebook#617
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 underscripts/
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