-
Notifications
You must be signed in to change notification settings - Fork 30k
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
debugger: also exit when the repl emits 'exit' #2369
Conversation
this.repl.rli.on('SIGINT', exitDebugRepl); | ||
|
||
// Exit debug repl on '.exit' | ||
this.repl.on('exit', exitDebugRepl); |
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.
Maybe prefer 'close'
? Does the same thing but I think 'exit'
may be an older event we don't use internally.
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 still used in L762, or am I missing something?
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.
Oh, hmm. Carry on then. Maybe we should switch it eventually.
@cjihrig @Fishrock123 ... does this look good to you now? |
LGTM |
// Exit debug repl on Ctrl + C | ||
this.repl.rli.on('SIGINT', exitDebugRepl); | ||
|
||
// Exit debug repl on '.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.
These comments aren't so correct imo. ctrl+c
isn't the only way to send a SIGINT
to a procees, and exit
can also be caused by ctrl+d
on an empty repl prompt.
LGTM otherwise |
Whoops... looks like this got the LGTM's but never landed. @Fishrock123 and @cjihrig ... still good with this? |
Yes, but let's see what the butler says. CI: https://ci.nodejs.org/job/node-test-pull-request/748/ |
This fails linting. There are some lines with trailing whitespace. If everything else is fine, it can be corrected when landing. |
A couple of unrelated errors in CI |
I will get this landed. @Fishrock123, when I land I'll also fix the comment re the |
Exit the debug repl when repl emits 'exit' Refs: nodejs/node-v0.x-archive#5637 Fixes: nodejs/node-v0.x-archive#5631 PR-URL: #2369 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in a95eb5c |
Exit the debug repl when repl emits 'exit' Refs: nodejs/node-v0.x-archive#5637 Fixes: nodejs/node-v0.x-archive#5631 PR-URL: #2369 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Exit the debug repl when repl emits 'exit' Refs: nodejs/node-v0.x-archive#5637 Fixes: nodejs/node-v0.x-archive#5631 PR-URL: #2369 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Exit the debug repl when repl emits 'exit' Refs: nodejs/node-v0.x-archive#5637 Fixes: nodejs/node-v0.x-archive#5631 PR-URL: #2369 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
See nodejs/node-v0.x-archive#5637, fixes nodejs/node-v0.x-archive#5631