Skip to content

feat: enable Corepack (pnpm support, unbundle yarn)#1768

Closed
dunglas wants to merge 2 commits into
nodejs:mainfrom
dunglas:feat/corepack
Closed

feat: enable Corepack (pnpm support, unbundle yarn)#1768
dunglas wants to merge 2 commits into
nodejs:mainfrom
dunglas:feat/corepack

Conversation

@dunglas

@dunglas dunglas commented Sep 7, 2022

Copy link
Copy Markdown

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, and pnpm, yarn and npm usable out of the box.

Example Output(if appropriate)

$ docker run -it node sh
# pnpm --version
7.11.0
# yarn --version
1.22.19
# npm --version
8.18.0

Types of changes

  • Documentation
  • Version change (Update, remove or add more Node.js versions)
  • Variant change (Update, remove or add more variants, or versions of variants)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Others (non of above)

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 time yarn is used. It wasn't the case previously.
I documented how to prevent this network access (for both yarn and pnpm).

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING.md document.
  • All new and existing tests passed.

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.
@dunglas

dunglas commented Sep 7, 2022

Copy link
Copy Markdown
Author

The CI failure is unrelated, it looks like the website generating the badge for the README is down.

@SimenB

SimenB commented Sep 7, 2022

Copy link
Copy Markdown
Member

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

@dunglas

dunglas commented Sep 7, 2022

Copy link
Copy Markdown
Author

@SimenB looks legit. Let me know when you'll want me to update the PR!

Alternatively, we could run corepack prepare in current images to prevent the backward compatibility break and remove it in v19?

@PeterDaveHello PeterDaveHello left a comment

Copy link
Copy Markdown
Member

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?

@dunglas

dunglas commented Sep 7, 2022

Copy link
Copy Markdown
Author

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 (sed will probably not be enough then)?

@merceyz merceyz left a comment

Copy link
Copy Markdown
Member

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.

@ljharb

ljharb commented Sep 29, 2022

Copy link
Copy Markdown
Member

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?

@arcanis

arcanis commented Nov 1, 2023

Copy link
Copy Markdown

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.

@ljharb

ljharb commented Nov 1, 2023

Copy link
Copy Markdown
Member

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?

@arcanis

arcanis commented Nov 1, 2023

Copy link
Copy Markdown

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.

@aduh95

aduh95 commented Nov 21, 2023

Copy link
Copy Markdown
Contributor

it seems like the TSC should have consensus on eventually enabling corepack by default in node itself

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.

@SimenB

SimenB commented Nov 21, 2023

Copy link
Copy Markdown
Member

Running corepack enable and installing yarn should be equivalent to what we have today, right? Except for pnpm command becoming available. Seems like an improvement on the current situation, plus corepack should get more usage in the wild (although user probably don't know it).

Could that be considered breaking in any way (discussions about pre-installing yarn/pnpm aside)?

EDIT: we could even start with just corepack enable yarn, that should be pretty invisible to end users (except missing env var for yarn version)

@dunglas

dunglas commented Mar 20, 2024

Copy link
Copy Markdown
Author

Is there anything I can do to help get this patch merged?

@SimenB

SimenB commented Mar 20, 2024

Copy link
Copy Markdown
Member

We should wait for whatever the resolution in nodejs/node#51931 and nodejs/node#51886 is

nodejs/TSC#1518

@cflee

cflee commented Apr 8, 2025

Copy link
Copy Markdown

The linked issue/PR have been closed, and the TSC has recently voted: nodejs/TSC#1697 (comment)

@eerison

eerison commented Jun 7, 2025

Copy link
Copy Markdown

is there anyway to enable pnpm without build a new image (https://github.com/satnaing/astro-paper/blob/main/Dockerfile#L6)?

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.

Separate variant including yarn

10 participants