-
Notifications
You must be signed in to change notification settings - Fork 172
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
base: main
Are you sure you want to change the base?
Conversation
I don't know why CI is failling at basic install step. Works fine locally for me |
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. |
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 I've seen cases where, for example, |
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. |
@Ethan-Arrowood is it something you're still working on? Is this block on something? |
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. |
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. |
Okay, I'll mark as ready for review now. And let folks review/approve/reject as they see fit. |
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 |
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.
Explicitly blocking; this needs tests to ensure it works.
@merceyz can you help me with the tests? I don't understand what to add where |
Hey friends, any update on corepack support for bun? Really looking forward to this. Thanks! |
@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. |
…orepack into feature/add-bun
Sorry for the delay. I've added more tests to match that other PR. |
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 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: Lines 4 to 8 in 12f1c31
|
I'm not sure I understand, so we need to find a different url for installing bun? Would their install script suffice 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. |
Work in progress - adding support for Bun. I filled out the json as best I see fit.
Investigating tests next
Closes #295