-
Notifications
You must be signed in to change notification settings - Fork 50
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
[dependencies-replacement] Replace spawndamnit with tinyexec (8 fewer dependencies) #224
[dependencies-replacement] Replace spawndamnit with tinyexec (8 fewer dependencies) #224
Conversation
🦋 Changeset detectedLatest commit: d9d1649 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
5ea95de
to
09ddfc8
Compare
packages/cli/src/index.ts
Outdated
let { code } = await spawn(args[0], args.slice(1), { | ||
cwd: pkg.dir, | ||
stdio: "inherit", | ||
const proc = exec(args[0], args.slice(1), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could u refactor the touched lines to this style (when possible)?
const proc = exec(args[0], args.slice(1), { | |
const { exitCode } = await exec(args[0], args.slice(1), { |
or at least to this if the .exitCode!
is problematic:
const proc = exec(args[0], args.slice(1), { | |
const result = await exec(args[0], args.slice(1), { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, the exec()
constructor and the return type of the await proc
are different types.
const proc = exec()
is an object that holds a "prepared execution" that you can attach streams/events/whatever on. It is also .then()-able, but that returns a different object...
const result = await proc
is a Result type that only has stdout
and stderr
. It doesn't have exitCode
(though I think it really should, there is an issue upstream, will see if I can open a PR for it).
The proc.exictCode!
should be correct, because exitCode
should always be set if we have awaited the proc
.
As weird as the code looks right now, it is the only way to write it as far as I can see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait a little bit then before landing this. It would be great if exitCode
thing would be improved upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was holding it wrong!
When the process has an non-zero exit code, an exception is thrown.
This means that if we don't read stdout/stderr, we can just do:
await exec("yarn", args.slice(1), {
nodeOptions: {
cwd: matchingPackages[0].dir,
stdio: "inherit",
},
});
And we can completely remove the ExitError
class, as tinyexec
throws its own NonZeroExitError
.
I'll update the PR and ping you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged the upstream change in tinyexec
: tinylibs/tinyexec#38
The PR is now good for re-review.
0d6ed8e
to
d9d1649
Compare
}); | ||
highestExitCode = Math.max(code, highestExitCode); | ||
highestExitCode = Math.max(exitCode ?? 1, highestExitCode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to treat a nullish exitCode
as a failure here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per node:child_proccess.spawn()
, exitCode
can be nullish in 2 cases:
- The process failed to spawn.
- The process was externally killed.
For our purpose, both of these mean that the underlying process never executed successfully, so we should treat a missing exit code as an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
First PR addressing #221.
https://github.com/tinylibs/tinyexec is used by
vitest
, so it should be a fairy safe bet.