throwOnError should default to true

[bluwy]

I'm a bit confused of the conclusion at #32. Isn't that the exec() should throw on non-zero exit code by default?

I just noticed this bug in Astro and for all its usage it should be throwing on error by default. It feels like the more sensible default to me.

[43081j]

The conclusion in the issue was that we add a throwOnError flag you're expected to set

Changing the default is a big breaking change I think. Id like to see more discussion here to know it's the right thing to do or not. If you know anyone relevant, please do cc them in

[bluwy]

From what I understand, in that issue #32 (comment), #32 (comment), and #32 (comment) support throwing on non-zero exit codes by default. Only #32 (comment) mention sticking with node's behaviour. And if throwOnError was false by default in the first place (and hence non-breaking), wouldn't the library not need to release a breaking v0 minor?

But anyways I think whether the option will be the default or not, I probably have to wrap all usages of exec() in Astro still to improve the error message. And I can set throwOnError: true along the way. But I still think true by default is more intuitive.

If we decide that we don't want to set true by default, it'd be nice if we put more emphasis on documenting the library difference/migration between execa or something else, in this repo or https://github.com/es-tooling/module-replacements. I've seen a lot of e18e efforts moving to lighter-weight libraries and introduce regressions often.

[43081j]

I'm open to setting the default to true, I just wanted some more opinions if possible since we're the only two people here and tinyexec has a lot of users now

But I'm not sure who best to pull in

I feel like most of the big consumers are ones we migrated at least, so should be easy to update

But it would be very helpful to get an idea of how many consumers check the exit code right now instead of throwing (i.e can we get at least one example of a consumer that isn't prepared for us introducing exceptions)

[bluwy]

Here's some I searched around that could benefit from throwOnError being false:

• https://github.com/nuxt/examples/blob/7ee6d47623c80579edd70549fae7c6f48acd7028/.scripts/typecheck.mjs#L20
• https://github.com/bluwy/auto-branch-sync-action/blob/a72b09dc60911f56a1e7093a069f24480ca3d2ab/index.js#L73-L75
• https://github.com/egoist/tsup/blob/9cc86d6d5bd8c1c75b9f6d4a74862b93d9fc02af/src/index.ts#L367-L377
• https://github.com/Thinkmill/manypkg/blob/4c32679951b1899b35f1a180934634b167758014/packages/cli/src/index.ts#L96-L102

But given those, there's also a fair share of code that is missing handling exit code 1, and throwOnError: true would had fixed them.

Personally, I think it's better to decide what the default value from an API design & ergonomics point-of-view rather than how it affects existing consumers and breaking changes, because adoption is only going to keep getting higher from here 😅

But to be honest, I'd also prefer the error message to be a little better if we ended up pushing `true` by default for everyone, because currently it's quite hard to read. I hope it could look something like this (click to open)

NonZeroExitError: Process started with `pnpm run asdf` returned exit code 1
   ERR_PNPM_NO_SCRIPT  Missing script: asdf
      Command "asdfsg" not found.
  at ...
  at ...

Code:

import { exec } from 'tinyexec';

try {
	await exec('pnpm', ['run', 'asdf'], { throwOnError: true });
} catch (e) {
	throw e;
	// console.log(e);
}

Right now it returns:

x [Error]: Process exited with non-zero status (1)
    at ...
    at ... {
  result: R {
    _process: ChildProcess {
      ... hundreds of lines of stuff related to child process
    }
  },
  output: {
    stderr: '',
    stdout: ' ERR_PNPM_NO_SCRIPT  Missing script: asdf\n' +
      '\n' +
      'Command "asdf" not found.\n'
  }
}

[43081j]

Of course 👍 we are deciding based on API design, not existing consumers. But we need to know about existing consumers so we can get them to update their usage (or we do it for them)

Anywhere a person currently does not try/catch and checks the exit code will need to update

I'm in agreement I think about changing the default but we will need to help update various projects