Have non-zero exit code throw for consistency with ezspawn and execa

[benmccann]

See https://github.com/sindresorhus/execa/blob/main/docs/errors.md#exit-code

And https://github.com/JS-DevTools/ez-spawn/blob/e7b51d8693be9b5ccedbe7eac7f8f7dd81b59ea2/lib/process-error.js#L9 / https://github.com/JS-DevTools/ez-spawn/blob/e7b51d8693be9b5ccedbe7eac7f8f7dd81b59ea2/lib/normalize-result.js#L31

[ericallam]

Could this be an opt-in behavior, maybe by exporting a separate function that does the throwing behavior?:

import { x, xSafe } from 'tinyexec';

// This throws by default
try {
  const result = await x('ls', ['-l']);
} catch (e) {
  // handle error
}

// this will not throw
const result2 = await xSafe('ls', ['-l']);

[43081j]

i've opened #34 to try tackle this in a breaking change, but im still unsure too @ericallam

ben is right here that existing libraries all seem to throw when a non-zero exit occurs. so migrating to tinyexec is a pain (or ends up buggy because people forget to try/catch it)

i'd like to avoid having a flag or two exports to switch this behaviour as its an easy way to go down the same bloated rabbit hole the other libraries went. we should decide if we want to throw, or if we don't, and it is what it is imo

[SukkaW]

IMHO, we could probably use the option nothrow:

x('ls', ['-l'], { nothrow: true })

And make it default to true to prevent breaking changes.

[benmccann]

I'd probably say that an exception should be thrown by default and released as 0.3.0. I think probably every consumer of tinyexec is currently not handling non-zero exit codes correctly as they've all migrated from ezspawn and execa where exceptions were thrown. And it's going to be a more annoying to migrate other packages from those libraries in the future if we have to check the exit code and then create and throw a custom exception to match the behavior.

I like the nothrow API suggestion if we do want to make it optional. Though it feels like that would only make sense if it an exception is thrown by default because nothrow: false is a double negative, which would be a bit weird. If it we were not throwing an exception by default, we'd probably want it to be throwOnFailure: true or something like that - though it's admittedly not as short and sweet as nothrow.

[ayuhito]

If it we were not throwing an exception by default, we'd probably want it to be throwOnFailure: true or something like that - though it's admittedly not as short and sweet as nothrow.

I quite like throwOnFailure because it is direct and clear. I agree double negatives are not great.

Maybe strict: true throws errors by default? Don't love it as much because it has less clear naming.

[43081j]

does anyone have a good example of why we wouldn't want to throw?

it does mean you have to access the process through err.process or some such thing

e.g.

try {
  await x('foo');
} catch (err) {
  err.process; // a tinyexec process
}

im not sure if there's much benefit to having a flag. you can achieve everything whichever way we do it, so i'd rather just go all in on whatever we decide on rather than making a new option

[ericallam]

@43081j in typescript this is a real pain. tinyexec would have to export the error type so you can do a error instance of ProcessExitError or something like that, to be able to get acceess to err.process. I think this library should just match how Node.js works and have a specific section in the README for people converting from execa, with a recipe for how to wrap tinyexec and throw your own custom error if you want that behavior.

[43081j]

i think you're right

@benmccann maybe a flag is right after all, a throwOnError or something and we just set that in things we're migrating from execa?

currently it throws when a command fails catastrophically (e.g. if it just doesn't exist or node was unable to execute it at all), and non-zero exits just resolve like any other exit. so we may have bugs out in the wild where we've migrated and expected a throw but didn't get one

[benmccann]

Okay. throwOnError seems like a solution that everyone can be happy enough with

[43081j]

added in 0.3.0 👍

[bluwy]

I've opened #39 to discuss whether throwOnError should be true (which I believe it should as to me it's the more common usecase/expectation). If anyone has thoughts about it, please comment in the issue!