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

nodemon support #258

Closed
wants to merge 1 commit into from
Closed

nodemon support #258

wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

This adds nodemon support for the SIGUSR2 handler, as described in #253 (comment).

@codecov-io
Copy link

Codecov Report

Merging #258 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage   74.26%   74.37%   +0.11%     
==========================================
  Files          17       17              
  Lines         882      886       +4     
==========================================
+ Hits          655      659       +4     
  Misses        227      227
Impacted Files Coverage Δ
src/cli.js 60.5% <100%> (+1.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e121cd4...d0d52d7. Read the comment docs.

@rauchg
Copy link
Member

rauchg commented Jan 31, 2019

I'm ok with supporting this if we document in README? Unless it's a standard that I don't know about. cc @adamelliotfields

@guybedford
Copy link
Contributor Author

So as far as I'm aware Node.js has no "generic" signal handling - all signals need to be handled individually by name, which does seem quite restrictive unfortunately.

In theory, we could support all signals as exit indicators (http://man7.org/linux/man-pages/man7/signal.7.html and I seriously considered this), but a good approach I think can be to expand this list based on user scenarios like this one. Unless anyone has other suggestions for how best to handle these signal responses generically.

@guybedford
Copy link
Contributor Author

Another altenative here might be just to remove this error case scenario entirely and always just act as if ncc run -f were the default.

@adamelliotfields
Copy link

adamelliotfields commented Jan 31, 2019

I'm with @guybedford for expanding the list based on user requests. Honestly, the only time I've encountered SIGUSR2 was with nodemon.

As for the docs, it would certainly be worthwhile to include instructions to users on how to gracefully handle signals. I'm using terminus which uses stoppable internally; but the way you handle it in serve is quite nice as well.

https://github.com/zeit/serve/blob/b7d9de3f647c78c581d8e9a6d33d4b63ab9cdbfb/bin/serve.js#L142-L155

Ideally, I'd like ncc to replace nodemon and ts-node so I can use the same tool in development and production 😄

@rauchg
Copy link
Member

rauchg commented Feb 1, 2019

I hear you. I'm still really on the fence about watch mode. I'm not convinced ncc is as fast as it can be yet when re-running it "cold".

@guybedford guybedford mentioned this pull request Feb 1, 2019
@guybedford
Copy link
Contributor Author

I've posted #265 which can supersede this.

@guybedford guybedford closed this Feb 1, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants