Open bluwy opened 4 days ago
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
From what I understand, in that issue https://github.com/tinylibs/tinyexec/issues/32#issuecomment-2304219134, https://github.com/tinylibs/tinyexec/issues/32#issuecomment-2305604410, and https://github.com/tinylibs/tinyexec/issues/32#issuecomment-2305872125 support throwing on non-zero exit codes by default. Only https://github.com/tinylibs/tinyexec/issues/32#issuecomment-2308417795 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.
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)
Here's some I searched around that could benefit from throwOnError
being false:
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 😅
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
I'm a bit confused of the conclusion at https://github.com/tinylibs/tinyexec/issues/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.