-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: fix --debug-brk interaction with -e #7089
Conversation
agent.stdout.on('data', (chunk) => { | ||
agentStdout += chunk; | ||
if (/connecting to .+ ok/.test(agentStdout)) { | ||
if (needToExit) { |
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.
Suggestion: fold this into the previous if statement like you do with the outermost if statement. Saves a level of indent.
LGTM. We have the means to fix the setTimeout hack now so that would be a good next step. |
LGTM |
The command line flag `--debug-brk` was ignored when the `-e` flag was also present. This change allows the flags to both be honored when they are used in a single command line. Fixes: nodejs#3589
The command line flag `--debug-brk` was ignored when the `-e` flag was also present. This change allows the flags to both be honored when they are used in a single command line. PR-URL: nodejs#7089 Fixes: nodejs#3589 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Landed in 1a21524 |
@Trott lts? |
@thealphanerd If it lands cleanly, yes. |
this one didn't land cleanly so sad 😿 |
Checklist
Affected core subsystem(s)
debugger
Description of change
The command line flag
--debug-brk
was ignored when the-e
flag wasalso present. This change allows the flags to both be honored when they
are used in a single command line.
Fixes: #3589
This changes some stack traces due to its modifications to lib/internal/bootstrap_node.js. Not sure if that's considered undesirable or not, but I imagine it could be modified to avoid that. See changes to expected output of messages tests to see what the changes are.
Not sure if this fixes a valid use case or not, but there's an issue open for it and there seems to be a history. @bnoordhuis surely knows more.
R=@bnoordhuis? @indutny?