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

Catch synchronous errors from spawning yarn #1204

Merged
merged 2 commits into from
Dec 8, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Dec 8, 2016

Maybe fixes #1200.
Apparently spawn sometimes fails synchronously and sometimes fails in an event emitter.
I restructured the code a tiny bit.

@gaearon gaearon requested a review from fson December 8, 2016 15:16
@gaearon gaearon added this to the 0.8.3 milestone Dec 8, 2016
if (yarnExists) {
callback(code, 'yarn', yarnArgs);
return;
}
Copy link
Contributor

@fson fson Dec 8, 2016

Choose a reason for hiding this comment

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

You need to call fallbackToNpm() here, so that we'll fall back to npm if the yarn process emitted the ENOENT error, which is asynchronous (handled above).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

var yarnProc;
var yarnExists = true;
try {
yarnProc = spawn('yarn', yarnArgs, {stdio: 'inherit'});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd only wrap this single line with try-catch, because that's the thing we need to catch the errors from. I don't want us to get stuck in an infinite loop if something else fails in the fallback npm command for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

@fson
Copy link
Contributor

fson commented Dec 8, 2016

Looks good to me 👍

@gaearon gaearon merged commit 270fe06 into facebook:master Dec 8, 2016
@gaearon gaearon mentioned this pull request Dec 8, 2016
@gaearon gaearon deleted the yarn-try branch December 8, 2016 15:49
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
* Catch synchronous errors from spawning yarn

* Fix issues
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
* Catch synchronous errors from spawning yarn

* Fix issues
@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.

Error while Installing react-scripts with create a new project
3 participants