Skip to content
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

Merged
merged 10 commits into from
Sep 7, 2024
Merged

Conversation

ziebam
Copy link
Contributor

@ziebam ziebam commented Sep 6, 2024

This PR is a suggestion to migrate from execa to a lighter alternative: tinyexec.

Description

  1. Most usage has been rewritten to use tinyexec which provides a wrapper around child_process's spawn.
  2. The sole usage of execaSync has been rewritten to use Node's child_process.spawnSync directly as tinyexec doesn't provide a wrapper around that. I haven't dug into it very deeply, but I've noticed a ~50% speedup in running trailer-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 of tinyexec 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. 😄

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link

codesandbox-ci bot commented Sep 6, 2024

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.

@escapedcat
Copy link
Member

escapedcat commented Sep 7, 2024

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?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 7, 2024

@escapedcat, thanks for taking a look! Should be fixed now. It seems that the stdin() function hung indefinitely as it waited for stdin and the stream was never closed. I assume that's because execa, even when provided with input = '' (as is the default when the test's cli is called without arguments), it "writes" that empty string to stdin anyway and explicitly closes the stream. tinyexec doesn't do that, so I got rid of the ifs checking for input.length > 0 in the places I've used.

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. 😄

@escapedcat
Copy link
Member

Thanks for fixing this!
Wondering why execa isn't being removed from the lock-file. Maybe I'm missing something?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 7, 2024

Thanks for fixing this! Wondering why execa isn't being removed from the lock-file. Maybe I'm missing something?

I think there are still downstream dependencies that use it? Namely: @lerna/child-process (execa@5.1.1), lint-staged (execa@8.0.1) and vitest (execa@8.0.1). vitest is migrating to tinyexec, but the change hasn't made it to the release yet (vitest-dev/vitest#6454), not sure about the other two.

@escapedcat
Copy link
Member

Got it, yes, makes sense. Thanks!

@escapedcat escapedcat merged commit 3c4b0fd into conventional-changelog:master Sep 7, 2024
7 checks passed
@ziebam ziebam deleted the execa-to-tinyexec branch September 7, 2024 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants