-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(core): add bun package manager #22602
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 631c414. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 6 targets
Sent with 💌 from NxCloud. |
if (packageManager === 'bun') { | ||
output.log({ | ||
title: | ||
"Unable to create bun lock files. Run bun install it's just as quick", |
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.
@edbzn Would it be worth calling stringifyYarnLockfile? But would have to edit how it is created into a file because it is a self-executable file
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.
My thinking is moving the following into a util
const lockFile = createLockFile(
builtPackageJson,
context.projectGraph,
packageManager
);
writeFileSync(
`${outDirRelativeToWorkspaceRoot}/${getLockFileName(packageManager)}`,
lockFile,
{
encoding: 'utf-8',
}
);
its duplicated code but run IF check for bun and use the bun CLI to generate the binary file
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.
Worth asking @meeroslav, he knows best how to deal with lockfiles
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.
We should reuse stringifyYarnLockfile
to parse the file, as long as it handles bun-specific strings: # bun
at the top, and version "workspace:<proj>"
if using bun workspaces.
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.
Since there doesn't seem to be a way to generate bun.lockb
without running the install, I would use the stringifyYarnLockfile
to be as close as possible to the intended package versions.
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.
I'm looking at generating the binary file. Happy to update snapshot so this can go in and work on that after? Also love to get involved in adding runtime and bun test. I have plugin that does it for now, but be nice if NX can add native support
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.
@jaysoo If you want to make the yarn.lock a follow up then I think this PR is completed?
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.
I reused Yarn in my PR bro. They still had problems with it. The people want Bun support!
If it builds and runs, then that's a start.
Help us brothers out for taking the time to do this for you.
It was my birthday btw when I built a working version for your project.
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.
This is different, this is to handle the manual creation of lock files not the reading of the lock file. Nx team doing a great job BTW, they been so many unstable issues. The chunk of this was taken from my PR back in Sep 11, 2023. But I closed it due to Bun having too many bugs
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.
@laynef you can always use nx-bun to get started for now
cc: @JamesHenry |
71efdf9
to
d0eb306
Compare
d0eb306
to
743444b
Compare
I was working in WSL. I'll fix the test in the morning on my Linux laptop |
743444b
to
b90d256
Compare
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.
Thanks a lot for this @Jordan-Hall!
We will need some e2e coverage in addition to the comments
4dd5910
to
99421c3
Compare
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.
Thank you for your work here! 👍
packages/workspace/src/generators/ci-workflow/__snapshots__/ci-workflow.spec.ts.snap
Outdated
Show resolved
Hide resolved
0c836bc
to
c27c614
Compare
I've had bun test working with NX it really good. Happy to help get a plugin in NX to support both runtime and tests. Would need guidance though by the team. I feel the node should turn into runtime and work with node and bun at least the generators |
@FrozenPandaz I think this is ready now. Let me know if you have any queries or any more issues :) |
c27c614
to
d424f93
Compare
d424f93
to
0f06c08
Compare
Yes, I'm on Discord. I'll take a look at the errors now and try to push a fix. |
Bun uses yarn lock for it's binary file. Running the binary will produce the content of a yarn lock file (v1) feat(core): update new generator schema for bun and fix workspaces setup for bun fix(core): update ci-workflow snapshot feat(core): check lock file for bunlock on push Signed-off-by: Jordan Hall <[email protected]> fix(core): add bun as a valid option of packageManager for preset generator fix(repo): make registry optional because of bun fix: handle where get registry is optional
Trying a run here to see if it'll go green #25066 Need to debug further on why CI is failing. |
Thank you. I'm around now on laptop if you need anything |
d2416d7
to
631c414
Compare
Yes yes yes @jaysoo merge before it goes again 🤣 |
Pushed a WIP PR to see how e2e tests are when we force Bun, since they are running with pnpm right now. If things are green or mostly green then we're good to go. |
Can I ask what the issue was with the ci :/ be good for future reference if I make anymore prs |
We'll need a follow-up to make sure that This is the code we need to fix: https://github.com/nrwl/nx/blob/master/packages/js/src/utils/package-json/update-package-json.ts#L106-L118. |
Happy to make that PR later |
Here's the PR :) |
You can try this now in canary.
|
First issue filed. Going to spend some hours debugging this and will post findings there.
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Bun uses yarn lock for it's binary file. Running the binary will produce the content of a yarn lock file (v1)
Other option is to use the -y command on add and install. This will create a yarn lock file and then createLockFile can just modify the yarn.lock file instead?
This is the PR made from #19113 and pushed due to #22402 being closed.
PS Bun feels more stable since the PR was first created!
This PR will resolve #22283 and start of #21075