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

Fix shape of exceptions rejected by execa() #276

Merged
merged 6 commits into from Jun 15, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Jun 6, 2019

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
Copy link
Owner

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

@sindresorhus sindresorhus reopened this Jun 6, 2019
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 6, 2019

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
Copy link
Owner

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
Copy link
Collaborator Author

ehmicky commented Jun 6, 2019

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
Copy link
Owner

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

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 6, 2019

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
Copy link
Owner

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 ehmicky changed the title Fix error thrown when options are invalid Fix shape of exceptions rejected asynchronously by execa() Jun 10, 2019
cp.catch(() => {});
cp.unref();
cp.on('error', () => {});
});
Copy link
Collaborator Author

@ehmicky ehmicky Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

ehmicky commented Jun 10, 2019

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

@ehmicky ehmicky changed the title Fix shape of exceptions rejected asynchronously by execa() Fix shape of exceptions rejected by execa() Jun 10, 2019
@ehmicky ehmicky force-pushed the feature/early-exception branch 2 times, most recently from 7eaa83b to c61bbef Compare June 11, 2019 09:03
@sindresorhus
Copy link
Owner

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
Copy link
Collaborator Author

ehmicky commented Jun 14, 2019

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.

@sindresorhus sindresorhus merged commit 49c74ac into master Jun 15, 2019
@sindresorhus sindresorhus deleted the feature/early-exception branch June 15, 2019 06:26
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.

Doesn't always return a ChildProcess
2 participants