Attach .nodeChildProcess to the execa promise

[sindresorhus]

Node.js does this now for childProcess. when you promisify it.

https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback

The name is to be decided.

One day, I hope we can get rid of execa() being both a childProcess and promise and just have the .child property on the promise. But that's a huge breaking change. For now, the best we can do is to add it, and recommend new users to use it instead of the old way. We could even un-document the old way.

@ehmicky Thoughts?

[ehmicky]

Yes this sounds good 👍

[sindresorhus]

Any thoughts on the name?

• .child
• .process
• .subprocess
• .childProcess

[ehmicky]

.child since this is the name used by Node.js util.promisify(childProcess.exec)?

[sindresorhus]

👍

[ehmicky]

@wesleytodd Based on your comment here, would you like to take a look at doing a PR for this feature? Please let us know if you need any help!

[wesleytodd]

I plan to, just have other priorities at work. If someone else gets to it before me that’s fine, my guess is I will be able to do it late next week at the earliest.

[ehmicky]

Per this comment, we are going to name the property subprocess instead.

[ehmicky]

We probably should keep the following specific methods on the returned promise, as opposed to the promise.subprocess property: .pipe() and [Symbol.asyncIterator]/.iterable(). That's because when those methods are called, the promise is awaited indirectly/differently. So those methods are more related to the promise than to the subprocess instance.

This leads a much more natural syntax.

const result = await execa`npm run build`
  .pipe`sort`
  .pipe`head -n 2`;

for await (const line of execa`npm run build`) {
  // ...
}

for await (const line of execa`npm run build`.iterable().drop(3)) {
  // ...
}

Instead of:

const result = await execa`npm run build`
  .subprocess.pipe`sort`
  .subprocess.pipe`head -n 2`;

for await (const line of execa`npm run build`.subprocess) {
  // ...
}

for await (const line of execa`npm run build`.subprocess.iterable().drop(3)) {
  // ...
}

All the other properties/methods though can be on the .subprocess property.

[ehmicky]

I've been thinking about this issue, and would suggest the following:

1. Execa-specific properties/methods (that do not exist in child_process, or have a different shape) remain available on the top-level promise returned by Execa: [Symbol.asyncIterator], .iterable(), .pipe(), .sendMessage(), .getOneMessage(), .getEachMessage(), .all, .readable, .writable, .duplex, .kill().
2. The following properties that do exist in child_process but are documented by Execa would remain available on the top-level promise, by copying them from the underlying ChildProcess instance: .stdin, .stdout, .stderr, .stdio, .pid.
3. The new .subprocess property would contain the underlying ChildProcess instance. This means it would contain all the other properties/methods which are neither documented nor recommended by Execa: .on() (i.e. all events), .send(), .channel, .connected, .disconnect(), .exitCode, .signalCode, [Symbol.dispose], .killed, .ref(), .unref(), .spawnargs, .spawnfile.

This means the return value of Execa:

• Before: is a mixin of Promise + Execa API + ChildProcess instance
• After: is a mixin of Promise + Execa API only, with a new property holding the ChildProcess instance

The reasons of this change is:

• Not to modify the underlying ChildProcess instance directly anymore.
• Not to turn a non-Promise (the ChildProcess instance) into a Promise anymore, by copying the Promise.* methods. That's a hack. Instead, assign the Execa methods/properties to a Promise, which is still a little hacky, but is much less problematic.

The upsides of keeping all the Execa API at the top level (as opposed to moving them to the new .subprocess property) are:

• This is "technically" not a breaking change (since we're only moving undocumented properties/methods), while moving it would be a major breaking change.
• This is more convenient. Having to type an additional .subprocess would be quite a setback in terms of experience IMHO, especially for methods like .pipe(), .iterable() or .sendMessage(), which become much more verbose. (see my previous message)
• This provides with a clearer distinction of documented vs undocumented API, where the underlying ChildProcess instance only acts as an escape hatch.

In other words, we're getting most of the benefits, but without the drawbacks. What do you think @sindresorhus?

[sindresorhus]

This generally sounds good.

Except, since we don't really want users to use the ChildProcess instance, maybe make it a less convenient name like .nodeChildProcess instead of .subprocess. The latter sounds like something that comes from Execa that we recommend using. .nodeChildProcess makes it clear we are just exposing the Node.js ChildProcess instance.

[ehmicky]

Yes, .nodeChildProcess sounds good. 👍

Also, this allows us to keep calling the top-level value subprocess, in the documentation, code and types.

So this leads to those action points:

• Add subprocess.nodeChildProcess property with the underlying child_process instance
• Top-level subprocess is not a child_process instance anymore. But it is still a promise.
• Keep all documented subprocess.* properties/methods.
  • For subprocess.stdin, subprocess.stdout, subprocess.stderr, subprocess.stdio and subprocess.pid, they must be copied from subprocess.nodeChildProcess.*.

So the main breaking changes are:

• When using any of the following properties/methods, subprocess.nodeChildProcess.* should be used instead of subprocess.*: .on() (i.e. all events), .send(), .channel, .connected, .disconnect(), .exitCode, .signalCode, [Symbol.dispose], .killed, .ref(), .unref(), .spawnargs, .spawnfile. We never documented those, but we did point users to the child_process documentation.
• Cannot use subprocess instanceof ChildProcess or Object.prototype.toString.call(subprocess) === '[object ChildProcess]' anymore. But subprocess?.nodeChildProcess instanceof ChildProcess can be used instead.

[ehmicky]

We probably should do the same change that we did with nano-spawn, i.e. make .nodeChildProcess a Promise<ChildProcess> instead of a ChildProcess. This would fix the same problems that are mentioned in sindresorhus/nano-spawn#43. Namely:

• On Windows, instead of doing 20-50 fs.statSync() calls serially before each command, parallelize it.
• No more weird "early errors" being thrown when there is a failure during the spawning itself. Those errors are super odd: we build artificial subprocess instances, and do other unusual stuff. See https://github.com/sindresorhus/execa/blob/main/lib/return/early-error.js

Generally speaking, not being able to use async in all the pre-spawn logic has been an hindrance for years. It would be nice to solve this by making accessing .nodeChildProcess asynchronous.

[sindresorhus]

👍