Skip to content
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

Merged
merged 1 commit into from Oct 7, 2019

Conversation

coreyfarrell
Copy link
Member

@coreyfarrell coreyfarrell commented Sep 6, 2019

Completely bypass spawn-wrap unless overridden with this new option.

@coveralls
Copy link

coveralls commented Sep 6, 2019

Coverage Status

Coverage increased (+0.008%) to 99.789% when pulling 350cfbd on coreyfarrell:set-node-options into 408c1cb on istanbuljs:master.

bin/nyc.js Outdated
*
* Ref https://github.com/nodejs/node/pull/24065
*/
/* istanbul ignore next */
Copy link
Member Author

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.

Copy link
Member

@JaKXz JaKXz Sep 10, 2019

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? :)

Copy link
Member Author

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

@isaacs
Copy link
Collaborator

isaacs commented Sep 7, 2019

This is cool. When did the support for env.NODE_OPTIONS land?

@coreyfarrell
Copy link
Member Author

This is cool. When did the support for env.NODE_OPTIONS land?

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 --require so it is not possible to directly do:

process.env.NODE_OPTIONS = `--require ${pathWithSpaces}`

bin/nyc.js Outdated Show resolved Hide resolved
@isaacs
Copy link
Collaborator

isaacs commented Sep 10, 2019

I was thinking about the case where someone clears the environment variable before calling spawn, like spawn(process.execPath, ['foo.js'], { env: { NODE_OPTIONS: 'different' } }).

We could update nyc's wrap.js to munge ChildProcess.spawn kind of like how spawn-wrap does, but without writing a shim file, such that it always puts the environment variable back in.

Then the only hazard would be something like spawn('bash', ['escape.sh']) where escape.sh does NODE_OPTIONS= node foo.js, but that's enough of an explicit step that we should probably respect it anyway.

@coreyfarrell
Copy link
Member Author

I was thinking about the case where someone clears the environment variable before calling spawn, like spawn(process.execPath, ['foo.js'], { env: { NODE_OPTIONS: 'different' } }).

We could update nyc's wrap.js to munge ChildProcess.spawn kind of like how spawn-wrap does, but without writing a shim file, such that it always puts the environment variable back in.

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.

Then the only hazard would be something like spawn('bash', ['escape.sh']) where escape.sh does NODE_OPTIONS= node foo.js, but that's enough of an explicit step that we should probably respect it anyway.

Agreed. Also we can't control everything.

bin/nyc.js Outdated Show resolved Hide resolved
@coreyfarrell coreyfarrell changed the title feat: Add --set-node-options=true option feat: Add --set-node-options option Sep 23, 2019
@coreyfarrell coreyfarrell force-pushed the set-node-options branch 3 times, most recently from 98bc99b to bd148c1 Compare September 24, 2019 02:05
Completely bypass spawn-wrap unless overridden with this new option.
@coreyfarrell coreyfarrell changed the title feat: Add --set-node-options option feat: Add --use-spawn-wrap=true option Oct 7, 2019
@coreyfarrell coreyfarrell merged commit df4de4d into istanbuljs:master Oct 7, 2019
@coreyfarrell coreyfarrell deleted the set-node-options branch October 7, 2019 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants