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

feat(core): support bun as a package manager #22402

Closed
wants to merge 44 commits into from

Conversation

laynef
Copy link

@laynef laynef commented Mar 19, 2024

Current Behavior

Screenshot 2024-03-19 at 6 52 28 PM

Expected Behavior

Screenshot 2024-03-19 at 6 24 12 PM Screenshot 2024-03-19 at 6 23 55 PM

Related Issue(s)

Fixes #

@laynef laynef requested review from a team as code owners March 19, 2024 22:01
Copy link

vercel bot commented Mar 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview Mar 22, 2024 9:09pm

@laynef laynef changed the title release(nx): Bun support release(nx): Bun support without documentation Mar 19, 2024
@laynef laynef changed the title release(nx): Bun support without documentation release(nx): Bun support without documentation generated Mar 20, 2024
@laynef
Copy link
Author

laynef commented Mar 22, 2024

This is the bare minimum required to support Bun.js

@JamesHenry please take a look again. Some changes are required to get that green check with minimal effort. The file changes are as minimal as the CI checks will allow.

I can't cut all the things you want.
You want me to cut too much.
The CI will not allow it.

Please pass this through for Bun.js support on NX! 🚀

@laynef
Copy link
Author

laynef commented Mar 22, 2024

FYI I have updated the PR title to better match our conventions

👍

@laynef laynef requested a review from JamesHenry March 22, 2024 19:24
@laynef
Copy link
Author

laynef commented Mar 22, 2024

@JamesHenry I removed the git hook file. Reverted it.
My apologizes, confusion on two different GitHub views.... My answer was incorrect and I apologize.

@laynef
Copy link
Author

laynef commented Mar 22, 2024

It's green again. This would be greatly appreciated if we could merge and make a new release.

@laynef
Copy link
Author

laynef commented Mar 24, 2024

It's building and passing. Please support this!

Screenshot 2024-03-24 at 1 00 08 PM Screenshot 2024-03-24 at 1 00 30 PM

@laynef
Copy link
Author

laynef commented Mar 24, 2024

@JamesHenry It is my birthday today! 🎂 🎈
What ever you need to get this merged for the Bun.js community, I'd appreciate you a lot for that green check!

@@ -126,7 +133,6 @@ export function getPackageManagerCommand(
};
},
npm: () => {
// TODO: Remove this
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this, I did not ask you to remove the existing comment. I asked you to remove the copy of this comment you added fresh in your PR

@JamesHenry
Copy link
Collaborator

@laynef Happy Birthday!

I can appreciate your enthusiasm in wanting to land this great feature, but we need to take a measured approach.

So far all we have been able to really focus on is reverting the many changes you made to the codebase that are unrelated to supporting bun.

Once we are at the point where the PR no longer contains unnecessary refactors and changes, then we will need to add e2e test coverage for bun usage for Nx. We will not land major features in Nx without adding high quality e2e tests.

For the e2e tests, we will definitely need updates to e2e/workspace-create/src/create-nx-workspace.test.ts to cover creating new Nx workspaces with bun as the package manager, you can see previous examples with other package managers. However I will also speak to the team about potential other scenarios we would want to capture in new tests.

Thanks for a the hard work, and I will continue to help you get this over the line, but please respect my requests for you to revert unrelated to changes and compare your local behaviour to the CI and master branch. I think there are some issues with your local setup somehow that are causing things to behave differently.

@JamesHenry
Copy link
Collaborator

JamesHenry commented Mar 25, 2024

@laynef Speaking of your local set up and things behaving differently - are you running things through bun all the time? Maybe that's the issue?

Please try setting up your environment and running all commands per the https://github.com/nrwl/nx/blob/54d47805de2803de183ab1dee6e05e161efc6d41/CONTRIBUTING.md and see if things improve for you and let me know

Copy link
Contributor

@edbzn edbzn left a comment

Choose a reason for hiding this comment

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

Hi, I just wanted to bring up that we should be careful with Nx features relying on the package manager, I remember facing issues with those when we upgraded Yarn to v4, so it's probably better to check that those features are working well with Bun. Here are the features I think about:

Regarding the lockfile, Bun has the capability to produce a lockfile in the Yarn format, a format already compatible with Nx. See the related docs.

@JamesHenry
Copy link
Collaborator

Thank you @edbzn, great callouts!

@JamesHenry JamesHenry added the PR status: do not merge This will block a PR from being merged until this tag is removed. label Mar 25, 2024
@JamesHenry
Copy link
Collaborator

JamesHenry commented Mar 25, 2024

I've spoken with the team and, in addition to the above comments, the major piece missing from this PR right now is the lock file parsing needed to truly support bun as a package manager.

You will need to add to the packages/nx/src/plugins/js/lock-file section of the codebase (where you can see existing parsers for the other package managers) and add an entry for bun.

Bun has a binary lockfile format, so you will not be able to parse it directly. My understanding is that we will instead want to run bun bun.lockb in a child process and capture the text representation of its output. Given, based on the bun docs, this should be compatible with the yarn v1 lock file format, we should hopefully be able to reuse a lot of the existing work done for yarn.

I also just want to add an overall disclaimer to this PR (particularly as I see you posting screenshots of running tasks via bun run):

It is only adding bun as an available package manager. It is not not handling full runtime support (e.g. the internals of how scripts are executed). Those will still be handled by node as of this PR.

Full runtime support can be handled in a follow up once this PR to complete bun package manager support goes in.

Hopefully that's all clear for now! Thanks again

@laynef
Copy link
Author

laynef commented Mar 26, 2024

Hi, I just wanted to bring up that we should be careful with Nx features relying on the package manager, I remember facing issues with those when we upgraded Yarn to v4, so it's probably better to check that those features are working well with Bun. Here are the features I think about:

Regarding the lockfile, Bun has the capability to produce a lockfile in the Yarn format, a format already compatible with Nx. See the related docs.

Ohh okay I see what you what is supposed to be supported. I can add that.
I'll copy Yarn's and make it Bun format.

@Jordan-Hall
Copy link
Contributor

Hi, I just wanted to bring up that we should be careful with Nx features relying on the package manager, I remember facing issues with those when we upgraded Yarn to v4, so it's probably better to check that those features are working well with Bun. Here are the features I think about:

Regarding the lockfile, Bun has the capability to produce a lockfile in the Yarn format, a format already compatible with Nx. See the related docs.

Ohh okay I see what you what is supposed to be supported. I can add that. I'll copy Yarn's and make it Bun format.

You can take a look at this PR it had the bun lock all sorted #19113 should be able to drop in to this PR without issue

@wall-street-dev
Copy link

Damn, we are soooo close. Also, Bun for Windows will be released in a couple of days!

@0xRapid
Copy link

0xRapid commented Apr 1, 2024

Bun is out! Lets go :)

@laynef
Copy link
Author

laynef commented Apr 2, 2024

Closing this. You guys figure it out

Copy link

github-actions bot commented Apr 9, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR status: do not merge This will block a PR from being merged until this tag is removed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants