Do not patch the global child_process module

[demurgos]

Hi,
I'm currently looking at providing a Node API for c8, a code coverage tool using V8's Profiler.
The current code uses spawn-wrap. Since spawn-wrap modifies the global child_process module, it is not safe to use it in a library: concurrent code trying to spawn processes will have unexpected results. This prevents running multiple coverages in parallel, or coverage and some other consumer tasks, inside the same Node process.

The current workaround would be to run each instance of c8 in its own process (independent child_process). This is very heavy-handed: it introduces friction and hurts composability. It means that the API must communicate with a subprocess so only serializable messages can be sent.

I propose to do something similar to graceful-fs@4. The version 3 modified the global fs module while version 4 only returned a wrapper.

Here is what the updated API would look like:

// Default to safe version (breaking change)
module.exports = safeWrap;
// Alias so the API can be a simple namespace
module.exports.wrap = safeWrap;
// Keep the old behavior available to ease transition, eventually mark as deprecated?
module.exports.wrapGlobal = wrap;
// Helper function to know if the global child_process is currently wrapped
module.exports.isGlobalWrapped = ...;

safeWrap(options) {
  const spawn = ...;
  const spawnSync = ...;
  // Start with a subset of the `child_process` API,
  // eventually extend it to return a full `child_process`-like object.
  return {spawn, spawnSync};
}

// We could also imagine a function like "wrapify` similar to "gracefulify"
// You can pass it a `child_process`-like object and it would mutate it
// by adding the wrappers in-place.

As stated in the example, this would introduce a breaking change but I believe that it would bring better behavior. The old behavior would still be accessible so transition would be easy.
The breaking change would also be the occasion to require Node 4+: there are other comments/issues mentioning problems with older Node versions. It would also simplify the current feature-detection mechanisms. This is not my main point though, I'm just proposing it by the way
What is needed is a way to spawn concurrent process-trees that do not interfere with each other, without isolating each root in its own sub-process.

---

Edit: /cc @bcoe @isaacs @sindresorhus

[demurgos]

By thinking more about the use cases of nyc and c8, I was thinking that a better API would be to use an event emitter to handle spawned subprocesses rather than patching the child_process module. (#47 (comment))

This could be achieved with some light IPC between the root process and its descendants.

[demurgos]

I did some refactoring this week-end to reorganize the code from a single module to smaller chunks. It fixed some small issues, but especially allowed me to get a clear picture of how the module works by untangling some parts (especially the setup/shim creation from from the internals patching).
See demurgos/spawn-wrap#next. I currently dropped support for unmaintained Node versions (requires Node 6+) because of language support. Adding a transpilation step to emit ES5 code may restore support for Node 0.12+ on a best-effort basis (not supported officially, but should not break). Apart from requiring a higher Node versions, the API itself is still compatible.

Adding support for local wrappers should be possible soon. The only thing missing is to have mungers working on the cp.spawn and cp.spawnSync arguments, not the internal spawn options. I initially tried to reuse them by pulling a bit of code from the Node lib, but there are just too much internal dependencies to copy here if we want to reuse the patch for the internal spawn functions. Working at the high-level functions means that we need to support the complex signature of these functions but it allows to wrap custom child_process implementations later one.

The next feature is term of difficulty would be to run the main module and wrapper in their own processes. This would fix the issue where the wrapper's flags are leaking into the original main (for example --require foo/register or when a process.exit in the original main prevents the wrapper from completing.

Finally, the event-emitter based API is still a goal for me. This is the best API responding to the problem solved by this lib. It's also the hardest feature to implement. It will require to add some IPC (as mentioned) and a class acting as a ChildProcess proxy. The wrapper would spawn the original main and forward events and calls between the spawn process and the root.