watch: strip watch flags from NODE_OPTIONS in child process#62143
watch: strip watch flags from NODE_OPTIONS in child process#62143marcopiraccini wants to merge 6 commits intonodejs:mainfrom
Conversation
Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62143 +/- ##
==========================================
+ Coverage 89.65% 89.67% +0.01%
==========================================
Files 676 713 +37
Lines 206543 224097 +17554
Branches 39547 42313 +2766
==========================================
+ Hits 185184 200956 +15772
- Misses 13480 14944 +1464
- Partials 7879 8197 +318
🚀 New features to boost your workflow:
|
| let cleanNodeOptions = kNodeOptions; | ||
| if (kNodeOptions != null) { | ||
| const nodeOptionsArgs = []; | ||
| const parts = RegExpPrototypeSymbolSplit(/\s+/, kNodeOptions); |
There was a problem hiding this comment.
Ultimately, this sort of approach isn't robust enough.
$ mkdir 'test for --watch parsing'
$ echo 'console.log("hi!")' > 'test for --watch parsing/test.cjs'
$ NODE_OPTIONS='--require "./test for --watch parsing/test.cjs"' node -p 'process.env.NODE_OPTIONS.split(" ")'
hi!
[ '--require', '"./test', 'for', '--watch', 'parsing/test.cjs"' ]
^^^^^^^I suspect this is going to need to interact with the options parser in some way.
There was a problem hiding this comment.
Good catch.
Thinking about it, maybe adding a C++ binding that exposes ParseNodeOptionsEnvVar to JS would be more robust (being the same parser to interpret NODE_OPTIONS, in my undersanding) and then finltering out watch flags. Will amend this PR.
Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
|
Oh, there's a test failing on macOS. |
Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
Because of node/.github/workflows/test-shared.yml Line 167 in d179c7e PTAL |
When
NODE_OPTIONScontains--watch, the watch mode parent process spawns a child that inherits the sameNODE_OPTIONS, causing the child to also enter watch mode and spawn another child, creating an infinite loop of process spawning.This PR strips watch-related flags (
--watch,--watch-path,--watch-preserve-output,--watch-kill-signal) fromNODE_OPTIONSbefore passing the environment to the child process. This mirrors the existing logic that already strips these flags fromprocess.execArgv.Non-watch flags in
NODE_OPTIONS(e.g.,--max-old-space-size) are preserved.To properly handle quoted strings and backslash escapes in
NODE_OPTIONSvalues (e.g.,--require "./path with --watch in name/f.js"), the C++ParseNodeOptionsEnvVartokenizer is exposed to JavaScript via a newinternalBinding('options')method. This reuses the exact same parser that Node.js uses at startup, ensuring consistent behavior.Fixes: #61740
This is actually similar to #61838 but more "surgical", it just avoid propagating
watchflags.