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

Node/Yarn to Bun #624

Merged
merged 15 commits into from
Mar 11, 2024
Merged

Node/Yarn to Bun #624

merged 15 commits into from
Mar 11, 2024

Conversation

vincerubinetti
Copy link
Contributor

@vincerubinetti vincerubinetti commented Mar 5, 2024

Converts repo from using Node and Yarn to just using Bun, and upgrades packages (notably Vite -> v5).

https://twitter.com/bunjavascript/status/1751726306793046490

This is not absolutely necessary. In my testing, it saves some headache and time, but it's also possible it could cause a problem down the road. See the added note in the readme.

I think using Bun just as a package manager is very low risk, so at the very least we could replace Yarn with Bun. In my testing, starting from cleared caches, Yarn took 1 minute to install the frontend dependencies, and Bun took 6 seconds (where simply downloading the packages was the main bottleneck). This could save us time waiting for frontend tests to finish, and possibly some $$ if we were to ever go over GitHub Actions quota.

Using Bun as a Node replacement, again from anecdotal evidence, is also fairly low risk. They seem to have already covered the general, commonly used Node functionality, and have tested popular tools like Vite to make sure they work with Bun. In some edge cases though, Bun might not behave exactly as Node. But that is a bug in Bun, as they explicitly say that anything in Node should also work in Bun. The other reason it's low risk is that it shouldn't be difficult to go back to Node if there is ever a problem. The biggest risk, I think, is there being a problem, and the maintainers not being aware that it could be a Bun bug.

Using Bun as a bundler or test runner would probably be a bigger change, and could cause more problems, in my estimation, so that is left out of this PR.

Another caveat: Bun's support of Windows is still in progress. They say about 90% of Bun's test suite passes on Windows.

@kevinschaper If someone has a laptop that does NOT have node or yarn installed on it, I'd be interested to test to make sure that just installing Bun will work as expected. Theoretically, Bun should be the only installation requirement for the frontend after this.

Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for monarch-app canceled.

Name Link
🔨 Latest commit 239fb29
🔍 Latest deploy log https://app.netlify.com/sites/monarch-app/deploys/65eb959c2c82820008d66388

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.55%. Comparing base (4b88871) to head (239fb29).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #624   +/-   ##
=======================================
  Coverage   69.55%   69.55%           
=======================================
  Files          91       91           
  Lines        2927     2927           
=======================================
  Hits         2036     2036           
  Misses        891      891           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@glass-ships glass-ships left a comment

Choose a reason for hiding this comment

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

nice, looks good!

@glass-ships
Copy link
Collaborator

it's weird to me that tests were working before i removed the node install step... and that we need a setup bun step for every job

but hey, if it works i guess

@vincerubinetti
Copy link
Contributor Author

vincerubinetti commented Mar 5, 2024

Ah yes, the e2e tests will be failing because of a quite stupid problem between playwright and linting:

microsoft/playwright#23662
trivago/prettier-plugin-sort-imports#270

Still working on a workaround for that. This is not due to Bun though, this is ultimately a result of upgrading to Vite 5 and it warning that "type": "module" in package.json will eventually be required, and then adding that is causing the above errors.

@vincerubinetti
Copy link
Contributor Author

vincerubinetti commented Mar 5, 2024

it's weird to me that tests were working before i removed the node install step... and that we need a setup bun step for every job

but hey, if it works i guess

Setup Node wasn't needed on all those steps before because the environments that GitHub Actions run come pre-installed with Node (along with other common things like Python and Ruby I think?). They do not come with Bun... yet. I predict that they will come with it in the next year or so. So eventually all those setup steps will not be necessary.

@vincerubinetti
Copy link
Contributor Author

Poking @kevinschaper for review, especially regarding my fresh laptop comment above.

@glass-ships
Copy link
Collaborator

re: fresh laptop, I spun up a fresh, barebones Ubuntu Docker container and ran the following:

apt update -y && apt upgrade -y && \
apt install git curl unzip build-essential -y && \
curl -fsSL https://bun.sh/install | bash && source /root/.bashrc && \
cd home && git clone https://github.com/monarch-initiative/monarch-app && \
cd monarch-app && git checkout bun && make install-frontend && \
cd frontend && bun run dev

and that did in fact seem to work without error. I couldn't remember how to expose a port with a Docker container, so unfortunately I wasn't able to actually view the site. but i did get:

$ vite

  VITE v5.1.4  ready in 477 ms

  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose
  ➜  press h + enter to show help

which more or less confirms that just installing bun should be sufficient

@vincerubinetti
Copy link
Contributor Author

Nice, thanks for doing that. We can chat more about this tomorrow before merging.

@amc-corey-cox
Copy link
Collaborator

nvm can be used for version managing in Node, does Bun have a similar feature? I know that javascript tends to lean towards a model of staying towards the newest versions but it is nice to have that kind of control when things break. How does Bun support this?

@vincerubinetti
Copy link
Contributor Author

That is a good question @amc-corey-cox , from some research it looks like it Bun doesn't have a "version manager" yet, either built-in or third-party. Personally I am using v1.0.0, atm, which can be checked with bun --version like normal. We could specify a minimum or specific Bun version number in the readme, then the developer would (for now) have to go get that version manually somehow.

@vincerubinetti
Copy link
Contributor Author

vincerubinetti commented Mar 8, 2024

We can shelve this for now, if we want. I'm unsure if the benefit it provides outweighs the cost/risk (which is still minimal imo). Locally, Bun is much much faster, for me. But on other people's laptops, it may not be.

Also I might be doing something non-optimally. The cache step might be unnecessary anymore. I remember seeing some tweet (that I can't find now) by the Bun guy that the setup-bun gh action didn't cache anything because the time it took to store/restore cache actually took longer than bun just doing it without cache.

@vincerubinetti
Copy link
Contributor Author

@vincerubinetti
Copy link
Contributor Author

I just removed the cache step completely, and it runs significantly faster somehow:

https://github.com/monarch-initiative/monarch-app/actions/runs/8209866962 (cache miss, need to do cache-save before cache-restores)
https://github.com/monarch-initiative/monarch-app/actions/runs/8209901871 (cache hit, only need to do cache-restores)
vs.
https://github.com/monarch-initiative/monarch-app/actions/runs/8209932865 (no cache at all)

The time it takes to save node_modules cache (10s) plus the time it takes to restore it in each step (10s) takes WAY longer than just install it on every step.

I don't know how Bun is doing an complete install in less than a second... I'm going to ask in the thread I linked above how that's possible. But the proof is in the pudding: it's working.

@glass-ships
Copy link
Collaborator

That's incredible, holy crap!

Also re: installing specific version of bun, it's as easy as:

curl -fsSL https://bun.sh/install | bash -s "bun-v1.0.0"

for example, according to the docs

@kevinschaper kevinschaper merged commit 5c1d4a2 into main Mar 11, 2024
11 checks passed
@kevinschaper kevinschaper deleted the bun branch March 11, 2024 20:55
@vincerubinetti
Copy link
Contributor Author

Forgot to remove CACHE_PATH as part of this.

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