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(npm): use bin.sh for execute #3550

Closed
wants to merge 1 commit into from

Conversation

dalisoft
Copy link

@dalisoft dalisoft commented Jul 30, 2024

Summary

Reduce execution startup time

This should solve few issues related to bun and it has a bunx/npx support compared to #2359
This script, bin.sh executes only as later it will be replaced to actual binary file
This approach does not work on pnpm and does not reduce startup time. For npx it is faster but not much, for bunx it is marginally faster

Test Plan

Faster startup time, saving CPU-time and power for small and medium projects

Benchmark

Command for test: hyperfine --warmup 2 --runs 3 'bunx dalisoft/biome-rs-npm#v1.8.3' 'bunx @biomejs/biome' --export-markdown biome-npx.md

Command Mean [ms] Min [ms] Max [ms] Relative
bunx dalisoft/biome-rs-npm#v1.8.3 4.9 ± 0.1 4.9 5.0 1.00
bunx @biomejs/biome 45.3 ± 0.1 45.2 45.4 9.20 ± 0.13

This should solve few issues related to bun and it has a `bunx`/`npx` support compared to biomejs#2359

This approach does not work on `pnpm` and does not reduce startup time. 
For `npx` it is faster but not much, for `bunx` it is marginally faster
Copy link
Contributor

@arendjr arendjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like a risky change to me. My first questions would be, how does this affect Windows support and has it been tested with Yarn PnP?

@ematipico
Copy link
Member

  • Has it been tested on windows?
  • Has it been tested on MUSL operating systems?
  • What bun issues are you talking about? It would be great if you could point to an issue, or explain them.

packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
# Test if calling via `npx` or `bunx`
if "$(echo pwd)" | grep -q ".bun/install/cache"; then
IS_X_CALL=true
CACHE_DIR=$(bun pm cache ls)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CACHE_DIR=$(bun pm cache ls)
CACHE_DIR="$(bun pm cache ls)"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than formatting, it does not change nothing at code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it does if $(bun pm cache ls) returns a string that includes spaces. Basically all $() should be enclosed by double quotes.

packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
packages/@biomejs/biome/bin.sh Show resolved Hide resolved
@Conaclos
Copy link
Member

ESbuild uses only a postinstal script. I wonder if we could use it as an inspiration.

@dalisoft
Copy link
Author

@Conaclos Yes, that way could reduce startup time

@dalisoft
Copy link
Author

@arendjr

It feels like a risky change to me. My first questions would be, how does this affect Windows support and has it been tested with Yarn PnP?

Not, not tested yet with Windows and Yarn PnP

@dalisoft
Copy link
Author

@ematipico

Has it been tested on windows?

Not yet

Has it been tested on MUSL operating systems?

Not yet

What bun issues are you talking about? It would be great if you could point to an issue, or explain them.

I mean #2359 which did not worked bunx/npx but now working with bin.sh script

@dalisoft
Copy link
Author

It is not @biomejs bug. It is just reference to issue which (previously) i'm made PR for reduce startup time attempt bug that not worked with bunx or npx so this solution works (see oxc-project/oxc#2920 (comment))

@ematipico
Copy link
Member

Closing for now, as this wasn't tested on other OS and with other package managers.

Also, the bash code is more convoluted than the Node.js code, and I risk it could introduce more friction in terms of contributions.

Feel free to open a discussion to discuss this alternative, if you want, and why we should accept this change.

@ematipico ematipico closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants