-
Notifications
You must be signed in to change notification settings - Fork 6
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
Node/Yarn to Bun #624
Conversation
✅ Deploy Preview for monarch-app canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
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.
nice, looks good!
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 |
Ah yes, the e2e tests will be failing because of a quite stupid problem between playwright and linting: microsoft/playwright#23662 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 |
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. |
Poking @kevinschaper for review, especially regarding my fresh laptop comment above. |
re: fresh laptop, I spun up a fresh, barebones Ubuntu Docker container and ran the following:
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:
which more or less confirms that just installing bun should be sufficient |
Nice, thanks for doing that. We can chat more about this tomorrow before merging. |
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? |
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 |
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. |
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) 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. |
That's incredible, holy crap! Also re: installing specific version of bun, it's as easy as:
for example, according to the docs |
Forgot to remove |
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.