-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
chore(deps): replace execa
with tinyexec
#6454
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
await execa(command, allArgs, { | ||
stdout: 'inherit', | ||
stderr: 'inherit', | ||
await x(command, allArgs, { | ||
nodeOptions: { | ||
stdio: ['pipe', 'inherit', 'inherit'], | ||
}, |
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.
tinyexec
doesn't provide a wrapper that passes the options along to stdio
, so we pass them directly. pipe
is used for stdin
(first item in the array) as it was the default in execa
: https://github.com/sindresorhus/execa/blob/HEAD/docs/api.md#optionsstdin.
return resolve(cwd, result.stdout) | ||
return resolve(cwd, result.stdout.trim()) |
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.
execa
trims the leading newline by default, while tinyexec
doesn't. Without trimming, we'd try to launch git
with invalid cwd
(due to the leading newline) which broke the functionality.
cwd: root, | ||
stdio: 'pipe', | ||
}, | ||
throwOnError: false, |
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.
From what I've investigated in both execa
and tinyexec
, this should be roughly equivalent to reject: false
. Relevant PR: tinylibs/tinyexec#34.
const child = x(typecheck.checker, args, { | ||
nodeOptions: { | ||
cwd: root, | ||
stdio: 'pipe', |
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.
stdio: 'pipe'
is equivalent to stdio: ['pipe', 'pipe', 'pipe']
which is the default in execa
.
execa
with tinyexec
execa
with tinyexec
That's just so you don't publish a random lockfile. If you update dependency, it's ok to update the lockfile. |
Thank you! |
Co-authored-by: Vladimir Sheremet <[email protected]>
Description
This PR replaces
execa
with a lightertinyexec
as discussed here: #5713. Some notes inline as a code review, please let me know whether it'd make sense to comment the code accordingly in those places.Not sure how to handle the third item in the checklist – should I revert the changes that remove the
execa
dependency?Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.