-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(core): improve application signal handler registration #5020
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
b62fdd4 to
2250d44
Compare
b9f4b42 to
ee0df1e
Compare
bajtos
left a comment
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.
This is much better than #5015 👏
I have few comments to consider, PTAL 👇
ee0df1e to
ec6a75a
Compare
6328a05 to
c7956ef
Compare
c7956ef to
3629439
Compare
|
@bajtos PTAL. |
3629439 to
4212146
Compare
bajtos
left a comment
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.
LGTM with one minor comment to consider. No further reviews are necessary as far as I am concerned.
emonddr
left a comment
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.
approve, but one question about a particular import statement.
- only register shutdown listeners when the app starts - unregister process listeners upon explicit stop - fix the test case to clean up process listeners - improve debugging with context name prefix
4212146 to
c53833d
Compare
Key changes:
Replace #5015
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈