-
Notifications
You must be signed in to change notification settings - Fork 466
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
setup: fail gracefully if package.json cannot be found #766
Conversation
this is ready for review ping @hoodiehq/maintainers |
bin/setup.js
Outdated
log.warn('setup', 'Could not find package.json at ' + path.join(pathToAppRoot, 'package.json')) | ||
log.warn('setup', 'You must manually set the start script in your app’s package.json to "hoodie" in order for "npm start" to work') | ||
|
||
process.exit(0) |
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 seem to be remembering a somewhat recent major version change in Node.js that process.exit
might cause console.log
messages to be swallowed because of some async operation. I think this note in https://nodejs.org/api/process.html#process_a_note_on_process_i_o refers to what I mean:
Synchronous writes avoid problems such as output written with console.log() or console.error() being unexpectedly interleaved, or not written at all if process.exit() is called before an asynchronous write completes. See process.exit() for more information.
and from https://nodejs.org/api/process.html#process_process_exit_code
It is important to note that calling process.exit() will force the process to exit as quickly as possible even if there are still asynchronous operations pending that have not yet completed fully, including I/O operations to process.stdout and process.stderr
Now the question here is if log.warn()
is sync or async.
If they are, I think the solution is to register with the beforeExit
event on process
and do the desired output there: https://nodejs.org/api/process.html#process_event_beforeexit
And a matter of style, if exit
ing because of an error condition, a return code other than 0
(zero) should be used. 1
would be an option.
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.
it’s not an error, the install already happened. It’s only the automated setup which fails and we log instructions on how to manually do it instead, so I thought warning fit better?
log.warn()
is synchronous, we don’t write any file, I think this should be fine
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.
we don’t write any file
stdout is a file :)
log.warn() is synchronous
What does it use under the hood?
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.
looks like they use process.stderr.write
(https://github.com/npm/npmlog/blob/master/log.js#L12, https://github.com/npm/npmlog/blob/master/log.js#L266)
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 didn’t know that console.log
s or process.(stderr|stdout).write
s could potentially not finish if we call process.exit(), I think I do that quite a bit. I’d love to learn what the proper way to handle this is. I’m not sure if a beforeexit
event handler will help because the documentation says
The 'beforeExit' event is not emitted for conditions causing explicit termination, such as calling process.exit()
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 misread the beforeExit
, sorry!
https://www.nczonline.net/blog/2014/02/04/maintainable-node-js-javascript-avoid-process-exit/ explains the issue.
nodejs/node-v0.x-archive#8329 (comment) Suggests wrapping the process.exit()
call in a process.nextTick()
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.
okay I read trough nodejs/node#6456 this is really complicated, now I understand npm’s frustration around Node@6’s release.
I pushed another commit to workaround using process.exit()
altogether:
182a818
7dd3e37
to
b0b3215
Compare
182a818
to
ee5a5a8
Compare
fixed via 79dde6b |
closes #751, follow up for #752