process.binding('spawn_sync') is required for spawn-wrap (and thus nyc) to work.

[coreyfarrell]

spawn-wrap works by intercepting calls to spawn and spawnSync. spawn is handled by replacing ChildProcess.prototype.spawn, spawnSync is handled by replacing process.bindings('spawn_sync').spawn. With #22160 and #22260 I assume we will need a migration path in the future.

CC @isaacs @bcoe

• Version: future
• Platform: all
• Subsystem: child_process

[sam-github]

@coreyfarrell nyc is important, do you have any suggestions as to what core could do to enable spawn-wrap? Would simply making sure that the binding's spawn was available somewhere be a good idea, or can you propose a more obvious API for hooking the underlying spawn? Also, do you know how/if coverage is done by any other coverage tools? I'm wondering if nyc has alternatives, and what they do.

Being able to implement code coverage metrics from tests is a pretty important quality of a language run-time, I'm wondering if something more maintainable can be proposed that would help multiple coverage providers.

[sam-github]

/to @nodejs/tooling

[coreyfarrell]

We hook require('child_process').spawn by replacing require('child_process').ChildProcess.prototype.spawn with a wrapper function. So I guess that could be a concern since ChildProcess.prototype.spawn is not documented?

spawnSync is what I'm concerned about (sorry my initial post said spawnWrap). Right now lib/internal/child_process.js directly exports spawnSync, if it were instead a static function of ChildProcess maybe we could wrap that. Not sure if that's a desirable API, I haven't tested to verify this will work yet as I don't want to try patching node.js without getting feedback (though from reading the code I think it should work). Replacing require('child_process').spawnSync will not work as we need our function to be called after the public spawnSync sets all the proper options.

c8 works by setting process.env.NODE_V8_COVERAGE, as long as nothing else touches that environment it'll capture all sub-processes. I'm not sure how other coverage systems handle sub-processes. The point of spawn-wrap is to allow nyc to add require hooks to all node.js child processes (and grandchildren, etc). So the ability to run nyc npm test depends on this as we don't control the way that npm spawns processes.

[sam-github]

One of my hopes about having a NODE_OPTIONS that supports --require is that it could be used to cause js to be loaded into all child node processes in a way that didn't involve monkey patching.

Would this be a mechanism you could use? Broadly, it seems to have the same effect as prepending an alternative main js file in order to do pre-setup, as nyc is doing now.

[coreyfarrell]

NODE_OPTIONS is used by end users and potentially by other libraries which perform spawns that we might want to capture results of. I think the problem with NODE_OPTIONS is that if something/someone wants to set it, they will replace the value entirely. You'd have situations where child/grandchild processes did not load the nyc hooks and covered silently dropped.

In theory a NODE_REQUIRES variable could be reliably useful if it were documented that the value should not replaced but instead prepended like PATH. Maybe even an API like process.hookChildren('@babel/register') and document that the variable should not be manually manipulated?

For either approach to a hypothetical nyc/register we would probably need to point the require to the absolute path of nyc/register to deal with situations where nyc is globally installed or otherwise in a place that is unresolvable to the target binary.

[sam-github]

I think the problem with NODE_OPTIONS is that if something/someone wants to set it, they will replace the value entirely

Not necessarily, they could append. It doesn't seem so different from PATH. It can be replaced, or appended to, and all kinds of carnage can occur if its done wrongly.

NODE_OPTIONS isn't set that much by node packages as they spawn children, because args can be put into ARGV more directly. Its mostly used in deployment before calling node or npm start, and for projects that do set it programmatically (in their unit tests, no less) to be told "that is breaking coverage, please append", doesn't seem so outrageous. After all, its always possible to break coverage. For example, by overwriting the spawn hooks you monkey patch in. Duelling spawn monkey patches seems only marginally less likely than dueling NODE_OPTIONS.

Maybe even an API like process.hookChildren('@babel/register') and document that the variable should not be manually manipulated?

An API like that, maybe using a "don't touch this directly" NODE_PRELOAD env variable that is essentally NODE_OPTIONS but only supports --require, seems not so hard to implement, but I'd need to gather a few more use-cases to be able to show its not just a nyc hack.

we would probably need to point the require to the absolute path of nyc/register

Yes. I assume you do that already with the startup runner you insert into the ARGV, or a process.chdir() would beak coverage.

[coreyfarrell]

One concern I have is escaping in NODE_OPTIONS. I'm considering adding an option to nyc@15 which would allow it to bypass spawn-wrap and prepend NODE_OPTIONS instead. My concern is that I'm not sure what is required to ensure proper/safe escaping of the NODE_OPTIONS value. I'm not sure it's necessarily a security concern but I do want to ensure that nyc works properly even if process.cwd() contains spaces or quote marks.

[bnoordhuis]

@coreyfarrell Backslash escapes the next character and text in double quotes is parsed as a single string. E.g. --arg="foo \"bar\" baz" works as you expect it to.

[coreyfarrell]

It looks like the option to --require cannot be quoted before node.js 12?

[cfarrell@lt2 nyc]$ nvm exec 12 ./bin/nyc.js --set-node-options ./mine.js 
Running node v12.10.0 (npm v6.10.3)
Success
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |     100 |      100 |     100 |     100 |                   
 mine.js  |     100 |      100 |     100 |     100 |                   
----------|---------|----------|---------|---------|-------------------
[cfarrell@lt2 nyc]$ nvm exec 10 ./bin/nyc.js --set-node-options ./mine.js 
Running node v10.16.3 (npm v6.9.0)
internal/modules/cjs/loader.js:638
    throw err;
    ^

Error: Cannot find module '"/usr/src/npm/nyc/lib/wrap.js"'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at Module._preloadModules (internal/modules/cjs/loader.js:901:12)
    at preloadModules (internal/bootstrap/node.js:601:7)
    at startup (internal/bootstrap/node.js:273:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------

From istanbuljs/nyc#1169 - not sure if you have any advice of how to properly escape/quote the path given to the --require option for node.js 8 / 10? Given my pwd of /usr/src/npm/nyc this is effectively initializing the environment with:

process.env.NODE_OPTIONS = ' --require="/usr/src/npm/nyc/lib/wrap.js"';

In the above test ./mine.js is an executable script that just runs console.log('Success');.

[coreyfarrell]

Nevermind, I found #24065 and the linked nearform PR, so I'm using NODE_PATH for node.js < 12 when the require path contains a space.

[coreyfarrell]

I discussed the current PR with @bcoe a few days ago, a concern we have with enabling the NODE_OPTIONS method of istanbuljs/nyc#1169 by default is require('child_process').spawn(bin, args, {env: {}}) - that child would not get NODE_OPTIONS, NYC_CWD or NYC_CONFIG environmental variables and thus no coverage.

Maybe even an API like process.hookChildren('@babel/register') and document that the variable should not be manually manipulated?

An API like that, maybe using a "don't touch this directly" NODE_PRELOAD env variable that is essentally NODE_OPTIONS but only supports --require, seems not so hard to implement, but I'd need to gather a few more use-cases to be able to show its not just a nyc hack.

I've created an experimental node-preload module that in theory could replace spawn-wrap. I realize this does not solve the process.binding issue as is but I think it could potentially be implemented in node.js core which would eventually eliminate the need for that module. I'd love feedback on the API. CC @bcoe @isaacs @JaKXz.

I realize this doesn't answer the question of "not just a nyc hack". In theory this could be used to implement a "all child processes" mode to @babel/register but I don't have absolute use cases.