-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: Add --use-spawn-wrap=true
option
#1169
Conversation
bin/nyc.js
Outdated
* | ||
* Ref https://github.com/nodejs/node/pull/24065 | ||
*/ | ||
/* istanbul ignore next */ |
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.
The branches following this line are version dependent. I have tested all three branches locally using node.js 8, 10 and 12. I've tested with nyc in a directory with and without spaces and double-quote.
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.
Does the ENOMEM error still occur on Windows with node 10+? Could we try re-enabling it in travis for coverage? :)
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 does still occur, specifically on 10.16.0 and above. Technically we could add 10.15.0
into testing.
See test log from yesterday when testing node-preload - https://travis-ci.org/cfware/node-preload/builds/582661358
This is cool. When did the support for |
node.js 8.0.0 added the option: https://nodejs.org/dist/latest-v8.x/docs/api/cli.html#cli_node_options_options node.js < 12 does not support quoting the path given to process.env.NODE_OPTIONS = `--require ${pathWithSpaces}` |
I was thinking about the case where someone clears the environment variable before calling spawn, like We could update nyc's Then the only hazard would be something like |
I thought of this, I've created https://github.com/cfware/node-preload as a potential alternative to spawn-wrap. The functionality has enough platform/version edge cases that it probably deserves it's own testing. I'm actually looking to get it to a state where it might be possible to get the functionality into the node.js core. My module is not published to npm yet, I want to get a little feedback first.
Agreed. Also we can't control everything. |
--set-node-options=true
option--set-node-options
option
98bc99b
to
bd148c1
Compare
Completely bypass spawn-wrap unless overridden with this new option.
bd148c1
to
350cfbd
Compare
--set-node-options
option--use-spawn-wrap=true
option
Completely bypass spawn-wrap unless overridden with this new option.