Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
src: whitelist new options for NODE_OPTIONS#13002
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
src: whitelist new options for NODE_OPTIONS #13002
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sam-github commented May 12, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
sam-github commented May 12, 2017
doc/api/cli.md Outdated
refackMay 12, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
You must have had a reason to leave those out on the first run?
They will make a very weird effect...inspect-port on the other hand is benign if used alone.
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.
Useful when running node via a shell script, and wanting to cause node when it runs to break on first line. I'm not sure how much thought went into this initially. I can leave only the -port if that's preferred.
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'm ±0 on these.
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.
ok, I'll give it a couple days to see if anyone feels strongly
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.
Seems useful to have, although if the node process spawns a child will that child also try to connect to the inspector?
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.
yes, so don't do that unless the port is 0
refack commented May 12, 2017
Ignore the fail on macOS (and maybe freeBSD) they are flakes #12964 (comment) |
Trott commented May 14, 2017
No idea if it would be difficult to accomplish or not, but a test for #12941 (assuming this fixes that issue) would be great. Or at least some kind of test for the functionality change here? |
sam-github commented May 16, 2017
refack commented May 16, 2017
If this lands before #12949 I can add tests for |
sam-github commented May 16, 2017
I removed support for --inspect, --inspect-brk, adding unit tests for every option passed via env var is a bit more than I can manage. I might remove support for --trace-event-categories as well, node has the unfortunate habit of randomly requiring either a |
39ccc51 to 2472f17Compare
refack 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.
Nit
doc/api/cli.md Outdated
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.
"--inspect-port"
0d3f956 to d6ce121Compare
mhdawson 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
d6ce121 to 42d7e61Comparesam-github commented May 17, 2017
sam-github commented May 17, 2017
Only failure was unrelated, sequential/test-net-connect-local-error on one of the FreeBSDs, I'm going to land this. |
Add --inspect-*, --napi-modules, --trace-event-categories Remove --prof-process, like -p and -e, it causes node to do something other than run node js scripts. PR-URL: nodejs#13002 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
42d7e61 to d6cd466CompareAdd --inspect-*, --napi-modules, --trace-event-categories Remove --prof-process, like -p and -e, it causes node to do something other than run node js scripts. PR-URL: nodejs#13002 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
sam-github commented May 30, 2017
See #12677 |
MylesBorins commented Jun 23, 2017
I've added don't land for v6.x If any of these flags should be backported please feel free to open a separate PR |
sam-github commented Jul 25, 2017
backported in #12677 |
Add --debug-*, --napi-modules Remove --prof-process, like -p and -e, it causes node to do something other than run node js scripts. PR-URL: nodejs#13002 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add --debug-*, --napi-modules Remove --prof-process, like -p and -e, it causes node to do something other than run node js scripts. Backport-PR-URL: #12677 PR-URL: #13002 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add --debug-*, --napi-modules Remove --prof-process, like -p and -e, it causes node to do something other than run node js scripts. Backport-PR-URL: #12677 PR-URL: #13002 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Add --inspect-port, --napi-modules, --trace-event-categories
Remove --prof-process, like -p and -e, it causes node to do something
other than run node js scripts.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src