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

[WIP] Add support for bun #307

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Ethan-Arrowood
Copy link

Work in progress - adding support for Bun. I filled out the json as best I see fit.

Investigating tests next

Closes #295

@Ethan-Arrowood
Copy link
Author

I don't know why CI is failling at basic install step. Works fine locally for me

@uncenter
Copy link

uncenter commented Sep 11, 2023

I don't know why CI is failling at basic install step. Works fine locally for me

Looks like those tests are trying to install yarn? Wonder how that could be your fault...

image

@merceyz
Copy link
Member

merceyz commented Sep 11, 2023

Node.js v20.6 changed the ESM loader a bit so the version of Yarn used by Corepack needs to be updated to a version containing yarnpkg/berry#5677.

Fixed by #308.

@merceyz merceyz mentioned this pull request Sep 11, 2023
@Jarred-Sumner
Copy link

Jarred-Sumner commented Sep 14, 2023

Would this be a symlink that does not execute JS in node? I think we should have a hard requirement that if Bun is to be included in Corepack, it must not incur the startup cost penalty of Node before executing bun (outside of the initial run to lazily install it)

I've seen cases where, for example, pnpm starts 2x slower due to using the executable via corepack instead of the pnpm executable

@arcanis
Copy link
Contributor

arcanis commented Sep 14, 2023

All calls to all commands have to go through Corepack, since it has to read the packageManager field to know the version of the package manager to use. It doesn't "cache" this information.

@aduh95
Copy link
Contributor

aduh95 commented Oct 6, 2023

@Ethan-Arrowood is it something you're still working on? Is this block on something?

@Ethan-Arrowood
Copy link
Author

Hey, no I'm not actively working on this anymore. It looks like we won't get sign off from Bun unless core pack is modified (#307 (comment)). I'm happy to help move this along if there is a clear path forward though.

@aduh95
Copy link
Contributor

aduh95 commented Oct 6, 2023

FWIW Corepack focuses on improving DX rather than performance, so IMO it wouldn't make sense to have a hard requirement as suggested in #307 (comment). Furthermore I don't think we need Bun permission (although if they feel strongly that we should not do that, it would be rude to not listen, but IIUC it's not what #307 (comment) is saying, please correct me if I'm wrong), so if the community wants Bun in Corepack, that's all that's needed (and passing CI of course). Contributions to improve Corepack performance are of course welcome, but it's not blocking as far as Corepack is concerned.

The path forward is to mark the PR as ready for reviews, and since the tests are already passing, get at least one approval and no objections.

@Ethan-Arrowood
Copy link
Author

Okay, I'll mark as ready for review now. And let folks review/approve/reject as they see fit.

@Ethan-Arrowood Ethan-Arrowood marked this pull request as ready for review October 6, 2023 18:53
@Ethan-Arrowood
Copy link
Author

I think this pr may need some tests of its own so if any core pack maintainers could help me with that. It would be appreciated

Copy link
Member

@merceyz merceyz left a comment

Choose a reason for hiding this comment

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

Explicitly blocking; this needs tests to ensure it works.

@Ethan-Arrowood
Copy link
Author

@merceyz can you help me with the tests? I don't understand what to add where

@oalexdoda
Copy link

Hey friends, any update on corepack support for bun? Really looking forward to this. Thanks!

@aduh95
Copy link
Contributor

aduh95 commented Jan 31, 2024

@merceyz can you help me with the tests? I don't understand what to add where

@Ethan-Arrowood you can use #333 for inspiration, I think we just need tests that validates the download succeeds and bun is able to run.

@Ethan-Arrowood
Copy link
Author

Sorry for the delay. I've added more tests to match that other PR.

@aduh95
Copy link
Contributor

aduh95 commented Feb 12, 2024

Bun package on the npm registry is relying on optional dependencies (https://www.npmjs.com/package/bun?activeTab=code). Because Corepack is not a package manager, it has no support for dependencies, so it doesn't work. Worth noting the bun binary tries to spawn a subprocess that expects npm to be on the $PATH, which seems problematic in the context of Corepack – I would consider it a bug if Corepack needed npm to be installed on the PATH to operate.

Support for Bun is still possible to implement into Corepack, but probably not from the npm package.

Note you also need to update the following list:

export enum SupportedPackageManagers {
Npm = `npm`,
Pnpm = `pnpm`,
Yarn = `yarn`,
}

@Ethan-Arrowood
Copy link
Author

I'm not sure I understand, so we need to find a different url for installing bun? Would their install script suffice https://bun.sh/install ?

I'm also going to re-emphasize that I really don't need to be driving this anymore. If this is more complex, I'd rather the Bun folks finish the swing here.

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.

Bun support
7 participants