-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Conversation
if (yarnExists) { | ||
callback(code, 'yarn', yarnArgs); | ||
return; | ||
} |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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'}); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
Looks good to me 👍 |
* Catch synchronous errors from spawning yarn * Fix issues
* Catch synchronous errors from spawning yarn * Fix issues
Maybe fixes #1200.
Apparently
spawn
sometimes fails synchronously and sometimes fails in an event emitter.I restructured the code a tiny bit.