Fix shape of exceptions rejected by execa()

[ehmicky]

Fixes #275.

When some options like uid are invalid, child_process.spawn() throws an exception.

At the moment, we catch that exception and return a rejected promise instead. This makes the return value polymorphic: it's probably a childProcess, but it might not be.

Instead we should keep the error synchronous, i.e. throw an exception just like child_process.spawn() does.

[sindresorhus]

Promise-returning functions should always reject, not throw: https://twitter.com/jaffathecake/status/638659818996269056

[ehmicky]

Yes what the W3C says makes sense.

Now the issue in our case is that we are returning both a promise and a childProcess. When child_process.spawn() fails to create a childProcess, how can we handle this?

At the moment, this requires every consumer to check childProcess.on !== undefined before using it.

[sindresorhus]

At the moment, this requires every consumer to check childProcess.on !== undefined before using it.

We should always add a .on function then. Even if it's just a noop.

[ehmicky]

What about the other properties?

If we go that way, it seems like we should set up a mock childProcess (with new ChildProcess()) and merge the error properties into it. That's a little ugly though.

[sindresorhus]

Hmm. I dunno. All I'm certain of is that it should never throw synchronously.

[ehmicky]

Yes I agree.

I also think that users that use the return value as childProcess (as opposed to a promise) should be able to rely on it having the childProcess properties and methods. Otherwise they either a) need to do extra checks, b) get bad exceptions that might be hard to debug.

The only thing I can think of is to merge new ChildProcess() and Promise.reject(makeError()). That's a little ugly, and I don't like it too much but I don't see another way.

Actually I think the core issue is us merging two core objects (promise and childProcess) into a single polymorphic one. That brings some nice features (await execa()) but it also comes with the usual drawbacks of polymorphism. An alternative would be childProcess.promise = promise instead of Object.assign(childProcess, promise). Or promise.childProcess = childProcess. But that would be too much of a breaking change (every user would need to update their code), so that's not a good idea.

What do you think of merging new ChildProcess() and Promise.reject(makeError())?

[sindresorhus]

Actually I think the core issue is us merging two core objects (promise and childProcess) into a single polymorphic one.

Yeah. That has been bothering me to, in hindsight.

Or promise.childProcess = childProcess.

Wish I had done that in the first place...

What do you think of merging new ChildProcess() and Promise.reject(makeError())?

👍

[ehmicky]

This test call methods from Promise, ChildProcess and EventEmitter. Just checking they exist is not enough. For example, this could be incorrect, calling those methods crashes even if they exist.

[ehmicky]

Fixed.

It turns out merging new ChildProcess() and Promise.reject() is not trivial, so I had to create a separate file for the logic. The reason is:

• we want to merge non-enumerable properties too (such as Promise.then())
• we want to merge prototype properties too (such as childProcess.on() taken from EventEmitter)
• non-configurable/writable/enumerable properties should remain so
• { ...object } and Object.assign() do not satisfy those requirements
• we do not want to modify prototype objects, since those are shared among all instances of that prototype

Basically this code achieves multiple inheritance (even though JavaScript prototype chain only supports single inheritance) by creating a new prototype object merging all prototypes.

Another approach would be to use Proxies like this code does.

I searched in GitHub for some code that does that and could not find it. The only repositories I can find are either:

• merging objects regardless of enumerability, prototypes, etc.
• complex/overkill OOP systems

Let me know if you want me to create a small module with that logic instead of adding it to execa, I'd be happy to create that module. Also if you find a module that already does that, let's use it instead, I don't like to reinvent the wheel :)

[sindresorhus]

Let me know if you want me to create a small module with that logic instead of adding it to execa, I'd be happy to create that module. Also if you find a module that already does that, let's use it instead, I don't like to reinvent the wheel :)

👍 Sure, but I think we should ship it for a while first to ensure it correctly solves all our problems. It's easier to extract it later when it's proven to work fine.

I would prefer using Proxy as it would be safer. I think the correct term for this is a "mixin".

[ehmicky]

I was thinking that the current PR had the following issue: the way the promise and child process properties are merged differ depending on whether an exception was thrown or not.

I re-implemented this PR by simply re-using the "merging" logic already present for the main return value. This turns out to be much clearer and error-prone, with no need for any weird mixin logic or module.

As a bonus, the then(), catch() and finally() methods of the return value of execa() are now non-enumerable, just like the corresponding Promise methods.