-
Notifications
You must be signed in to change notification settings - Fork 2k
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: enable Corepack (pnpm support, unbundle yarn) #1768
base: main
Are you sure you want to change the base?
Conversation
This patch enables Corepack, allowing to use pnpm directly and unbundling yarn from the default image. Removing yarn also simplifies the image and the maintenance. This change has been discussed in nodejs#777. Closes nodejs#777, nodejs#1645, nodejs#1755.
6602cc1
to
865b4b4
Compare
The CI failure is unrelated, it looks like the website generating the badge for the README is down. |
Exciting! I don't however think we can do this for existing images - it'd be a very breaking change. However, we can probably do this for v19 when it comes out next month and for all future versions? /cc @nodejs/docker |
@SimenB looks legit. Let me know when you'll want me to update the PR! Alternatively, we could run |
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.
Awesome, same concerns as @SimenB, maybe do it for future major release?
Do you have a preference on how to proceed? Should I keep the old templates and add new ones for future major releases or should I use the same templates and introduce conditions inside the template ( |
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.
Would need to set COREPACK_DEFAULT_TO_LATEST=0
to ensure the default versions remains stable and doesn't depend on when the image was built, also need to run corepack prepare --all
or yarn --version
(and pnpm --version
) as well to make sure environments without access to the npm registry still work.
Why would we want it enabled by default in docker if it's not enabled by default in node itself? Shouldn't those two happen at the same time, or not at all? |
I disagree, I don't think those two distributions have the same requirements. The general Node.js distributions are tarballs that users manipulate in many different and unpredictable ways, whereas the Docker images are fixed environments generally pinned by major version. Enabling Corepack in the Docker images first would be lower risk than enabling it by default in both Docker images and the general distribution, and would provide useful early signals. |
Perhaps, but it seems like the TSC should have consensus on eventually enabling corepack by default in node itself - otherwise, enabling it by default anywhere isn't useful, because no signals are needed. Does it have that consensus? |
It's worth double-checking when the time comes to enable it by default on all distributions, but I believe so; opt-in was suggested to derisk the project (nodejs/node#35398 (comment), nodejs/node#35398 (comment)), but the point was always to eventually make it the default once we had enough signals that it was working appropriately. That said, in the specific case of the Node.js Docker images, since Yarn is already distributed there, enabling Corepack would mostly be an implementation change (as long as the default Yarn binary is also made available without requiring the network). In that regards, for the Docker images only, it seems more up to the Release maintainers than the TSC - although I'm not part of either group, so that's just my understanding. |
FWIW changes in node are not up to the TSC but to Collaborators, who are the collective owners of the project. The TSC would be involved only if tentatives to reach consensus among collaborators have failed. |
Running Could that be considered breaking in any way (discussions about pre-installing EDIT: we could even start with just |
Is there anything I can do to help get this patch merged? |
We should wait for whatever the resolution in nodejs/node#51931 and nodejs/node#51886 is |
Description
This patch enables Corepack, allowing to use pnpm directly and unbundling
yarn
from the default image.Removing
yarn
also simplifies the image and the maintenance.Motivation and Context
This change has been discussed in #777.
Closes #777, #1645, #1755.
Testing Details
I build some images generated from the template (I'm using Mac OS X), and tested that Corepack is enabled,
yarn
unbundled, andpnpm
,yarn
andnpm
usable out of the box.Example Output(if appropriate)
Types of changes
To reduce the image size, I decided to not bundle
yarn
anymore. So there is a potential tiny breaking change: the network is used the first timeyarn
is used. It wasn't the case previously.I documented how to prevent this network access (for both
yarn
andpnpm
).Checklist