-
Notifications
You must be signed in to change notification settings - Fork 914
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: replace execa
with tinyexec
and Node's child_process.spawnSync
#4134
chore: replace execa
with tinyexec
and Node's child_process.spawnSync
#4134
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Thanks @ziebam ! I had a look at the issues/PRs related to vitest. Looks like one test is failing: ❯ @commitlint/cli/src/cli.test.ts (57 tests | 1 failed) 20862ms
× should throw when called without [input]
→ Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
FAIL @commitlint/cli/src/cli.test.ts > should throw when called without [input]
Error: Test timed out in 5000ms.
If this is a long-running test, pass a timeout value as the last argument or configure it globally with "testTimeout".
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
Test Files 1 failed | 83 passed (84)
Tests 1 failed | 1055 passed (1056)
Start at 08:47:41
Duration 22.33s (transform 3.14s, setup 0ms, collect 7.98s, tests 29.32s, environment 21ms, prepare 7.67s) Did all tests pass for you locally? |
@escapedcat, thanks for taking a look! Should be fixed now. It seems that the Sorry for that! I initially assumed the test failed because my machine isn't powerful enough (since it timed out), but it turns out I was wrong. 😄 |
Thanks for fixing this! |
I think there are still downstream dependencies that use it? Namely: |
Got it, yes, makes sense. Thanks! |
This PR is a suggestion to migrate from
execa
to a lighter alternative:tinyexec
.Description
tinyexec
which provides a wrapper aroundchild_process
'sspawn
.execaSync
has been rewritten to use Node'schild_process.spawnSync
directly astinyexec
doesn't provide a wrapper around that. I haven't dug into it very deeply, but I've noticed a ~50% speedup in runningtrailer-exists.test.ts
, so that's another gain related to this change.Motivation and Context
The migration improves the bundle size and installation time for the project, therefore improving the UX. Most of the advanced
execa
features are unused, and those that are, are simple to rewrite into a blend oftinyexec
and native functionality.Usage examples
N/A
How Has This Been Tested?
yarn test
Types of changes
Not sure what to pick here, please advise. 😄
Checklist: