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

deps: remove corepack #51981

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

darcyclarke
Copy link
Member

@darcyclarke darcyclarke commented Mar 6, 2024

"Package managers" manage packages & their versions. Package managers are packages themselves. This makes a "package manager version manager" redundant. Improvements can be made to the existing default tooling (ex. devEngines).

Users can still access Corepack via npm, npx (ex. npm i -g corepack or npx corepack) or source as they were prior to #39608.

Fixes: #51888
Related to: #51951

@nodejs/tsc @nodejs/corepack @nodejs/npm @nodejs/package-maintenance

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/security-wg
  • @nodejs/startup
  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels Mar 6, 2024
@ronag
Copy link
Member

ronag commented Mar 6, 2024

I mean if we are not removing npm then this is kind of a valid point...

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I don't think this is feasible, as there is massive user support behind having yarn and pnpm binaries.

However, I think you could implement the proposal in #51931, which would solve have the same user need without corepack.

@ronag
Copy link
Member

ronag commented Mar 6, 2024

I don't think this is feasible, as there is massive user support behind having yarn and pnpm binaries.

However, I think you could implement the proposal in #51931, which would solve have the same user need without corepack.

Isn't it possible to install both yarn and pnpn with npm?

@targos
Copy link
Member

targos commented Mar 6, 2024

Corepack doesn't only exist to install yarn and pnpm. It manages multiple installed versions and ensures the right one is used based on the current project.

@mcollina
Copy link
Member

mcollina commented Mar 6, 2024

Isn't it possible to install both yarn and pnpm with npm?

One command in a bash script/Dockerfile/instruction is too hard/burdensome for many users.

Corepack doesn't only exist to install yarn and pnpm. It manages multiple installed versions and ensures the right one is used based on the current project.

The perceived value for users is that it allows them to use yarn and pnpm without a specific install command. They are mostly oblivious to the problems of "multiple installed versions" and how to manage this safely.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

But they can't pin versions. I'm -1.

@darcyclarke
Copy link
Member Author

You can pin versions.

@GeoffreyBooth
Copy link
Member

One command in a bash script/Dockerfile/instruction is too hard/burdensome for many users.

So these users are sophisticated enough to use a package manager but too novice to install a package manager?

@aduh95
Copy link
Contributor

aduh95 commented Mar 10, 2024

I think I agree with what @mcollina meant, but I don't really agree with the phrasing in #51981 (comment).

IMO, it's more useful to think about the "least friction path":

  • without Corepack in the Node.js distribution, the path of least friction is to use a globally managed version of your package manager (either the provided npm binary, but another package manager installed by e.g. npm i -g yarn).
  • with Corepack in the Node.js distribution, the path of least friction (for non-npm project at least) is to pin package manager per project, effectively treating the package manager as a dev dependency. (And even more so since feat: Pins the package manager as it's used for the first time corepack#413 landed.)

Of course, whether treating a package manager as a dependency is desirable is up for debate (but let's try not to do that in this thread to keep the conversation about the proposed change), but from what I heard it's one reason that Corepack users have found compelling; and personally, I think the whole ecosystem would benefit from that pattern being more wide-spread (for the same reasons lock files were a good thing for the ecosystem).

Also, it does reduce some friction for users of Yarn and pnpm, which is also a positive IMO. (I reckon that point is debatable, but at least for Yarn Berry users it's pretty clear the UX is better with Corepack).

Certainly Corepack is not strictly necessary to achieve what its users are doing with it, it's not the only tool that can do it, and it's far from perfect. However Corepack users are still getting value out of it, and certainly it would not have gotten the same interest if it wasn't maintained within the project and distributed with Node.js.

TL;DR Having Corepack removes some friction for some of our users. Removing it adds friction, I don't think we should do that.

@ShogunPanda
Copy link
Contributor

ShogunPanda commented Mar 11, 2024

I'm -1 here as well.

Not applicable anymore. See #51981 (comment)

@GeoffreyBooth
Copy link
Member

Having Corepack removes some friction for some of our users. Removing it adds friction, I don’t think we should do that.

I don’t think this is really in dispute; the question is whether the reduced friction is worth the cost. We could bundle CoffeeScript, too, and that would also reduce friction for some of our users. We could bundle any number of things, some of which might arguably be more useful or serve more users; I think the general rules of what should go into core should apply to what goes into the bundle. Arguably those rules should mean we exclude npm, but npm got included long before those rules were written; and besides, each addition should be judged on its own merits.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

It’s not clear to me what goals Corepack aims to achieve, nor why inclusion in the Node bundle is necessary for achieving them. Therefore I think we should just remove it and spare ourselves the maintenance burden and security risk associated with Corepack, and users who wish to continue using it can install it directly, such as via npm install --global corepack or via the equivalent command in the package manager of their choice.

@ShogunPanda
Copy link
Contributor

After a careful thought I changed my mind. LGTM.

@ronag
Copy link
Member

ronag commented Mar 23, 2024

I think the DX is a good idea and wouldn't mind pursuing it. But this corepack question is taking too much effort.

I think there already are alternatives (though maybe not as user friendly)

You can install corepack:

npx corepack
or
npm i -g corepack

You can install a package manager

npm i -g yarn
or
npx yarn

You can pin a package manager version to your project.

npm i yarn@{version}
npm run yarn {command}

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I’m -1. corepack is part of the flow of many devs.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I changed my mind due to nodejs/corepack#495.

Corepack poses a significant security risk as it is.

@gurgunday
Copy link

I won't agree on removing corepack as long as there is no alternative

@BridgeAR, from what I understand, the main proposal isn't to remove corepack entirely from the node org, but to change how it's distributed; the suggestion is to keep it as a separate npm package, so users would run npm i -g corepack before using it — this approach seems more transparent to me than silently making corepack a runnable command

Is it really crucial that users skip this one-time installation step? We already have precedent for this with undici, which is npm-installed despite being maintained by the Node team [1] — corepack could be like that

Also, I agree with @mcollina that nodejs/corepack#495 makes it dangerous to include in the default installation; I think with the current level of integration with npm, the default node installation should only make requests to the npm registry/domain

[1] — I know it's also included in Node with fetch, but the whole package is not accessible by default

@SandersAaronD
Copy link

2 cents from mostly uninvolved party: I'm watching this PR because I have to modify CI/CD around yarn versioning if this happens. From my perspective it's a breaking change surrounding package mgmt that would have to make me re-evaluate using corepack at all. I would be surprised if I was unique.

I do think the security behavior concern is valid. As an outsider, I would vastly prefer to see corepack's security guarantees made tighter versus changing the contract around using it.

@filips123
Copy link

the suggestion is to keep it as a separate npm package, so users would run npm i -g corepack before using it — this approach seems more transparent to me than silently making corepack a runnable command

Shouldn't it be the opposite? IMO, it would make more sense for Node.js to stop bundling npm directly, and instead let corepack (which should be enabled by default) manage it and other package managers.

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

Changed my mind

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I no longer work for Microsoft and believe it's fine for me to take a position on this (officially).

@anonrig
Copy link
Member

anonrig commented Jul 28, 2024

I still think we shouldn't remove corepack

@ChALkeR
Copy link
Member

ChALkeR commented Jul 28, 2024

Looking at https://github.com/nodejs/node/blob/384fd1787634c13b3e5d2f225076d2175dc3b96b/deps/corepack/dist/lib/corepack.cjs (and the disruption its new behavior brings, and the security concerns), I'm in favor of removing it

I remember it being supposed to be a simple tool to help users install missing package managers, and that was the reason why it got accepted, I think?

Why is it doing all that now?

Could perhaps the cmd shims be kept and replaced with a wrapper on top of npm that we already ship? To prompt npm i -g {} on user confirmation, instead of all this?

Also could minimize the negative imact

@hildjj
Copy link

hildjj commented Jul 29, 2024

It would be nice for there to be a recipe for how to run node unit tests on GitHub Actions with [pnpm, yarn], without corepack, with caching enabled.

This currently works:

    steps:
      - uses: actions/checkout@v4
      - name: Install pnpm
        run: corepack enable
      - name: Use Node.js ${{ matrix.node-version }}
        uses: actions/setup-node@v4
        with:
          node-version: ${{ matrix.node-version }}
          cache: pnpm

Note that corepack has to be turned on before cache: pnpm will work, today.

@RobertWHurst
Copy link

This feels like if the rust team started thinking about dropping rustup. I believe this is likely the wrong move. In fact, corepack should take on more, not less of a role in the standard node setup. I was hoping to see it eventually take on node version management as well. Then we won't need so much 3rd party tooling to make node development reasonable when compared to other modern languages.

The gist is, I'm not sure why there's consideration to remove it - it seems like some steps backward.

@joshuajaco
Copy link

joshuajaco commented Jul 30, 2024

I've been using pnpm exclusively through corepack.
The main reason is npm has basically become unusable over the years. It is incredibly slow, often gives confusing error messages and sometimes just gives wrong, non deterministic results.
Forcing people to use npm to install the package manager they actually want to use is a terrible step backwards.

This PR introduces unnecessary complexity to any setup using corepack for no reason. If anything, corepack should be bundled by default instead of npm.

@elektronik2k5
Copy link

I'll also add my deep disappointment with this sad state of affairs. Lots of people choose not to use npm for lots of valid reasons. For this crowd, corepack has been nothing short of a lifeline, which this PR aims to sever. It'll be a terrible regression and will harm many node users. It is actively hostile and I wish more people realized that.

@benjamingr
Copy link
Member

My main issue with corepack hasn't been "pro npm", I think diversity and competition is inherently good and we should encourage it as the project. This PR moves corepack to a userland module that can still be maintained/vendored by the Node.js project, we can still have install instructions for it etc.

My issue has been that corepack is a tool that makes it easier to install official and unofficial npm registry clients. The public registry is still the npm registry and yarn/pnpm are still npm clients. (much like third party twitter clients or reddit clients). They exist at the grace of the npm registry who may choose (but hopefully won't choose to) block them.

There was work on yarn having its own registry but I suspect due to funding/time that didn't happen and the closest we had was a reverse proxy to the npm public registry.

I think moving corepack from experimental and keeping it in core would be meaningful if and only if we somehow fund/maintain the public registry the same way Python/Rust do but I suspect that's a lot of work and not on the table - so unless someone is willing to foot the bill, this whole conversation is inherently about npm registry clients for the official npm server.

@wesleytodd
Copy link
Member

I would encourage anyone who would like to discuss this to attend our team meeting today.

nodejs/package-maintenance#606 (comment)

Our meeting is in ~30min, please come and discuss.

@ovflowd
Copy link
Member

ovflowd commented Jul 30, 2024

My main issue with corepack hasn't been "pro npm", I think diversity and competition is inherently good and we should encourage it as the project. This PR moves corepack to a userland module that can still be maintained/vendored by the Node.js project, we can still have install instructions for it etc.

My issue has been that corepack is a tool that makes it easier to install official and unofficial npm registry clients. The public registry is still the npm registry and yarn/pnpm are still npm clients. (much like third party twitter clients or reddit clients). They exist at the grace of the npm registry who may choose (but hopefully won't choose to) block them.

There was work on yarn having its own registry but I suspect due to funding/time that didn't happen and the closest we had was a reverse proxy to the npm public registry.

I think moving corepack from experimental and keeping it in core would be meaningful if and only if we somehow fund/maintain the public registry the same way Python/Rust do but I suspect that's a lot of work and not on the table - so unless someone is willing to foot the bill, this whole conversation is inherently about npm registry clients for the official npm server.

Agreed with all statements. This allows us to better take care of Corepack without worrying about the implications of it being within Node core.

@styfle
Copy link
Member

styfle commented Jul 31, 2024

corepack is a tool that makes it easier to install official and unofficial npm registry clients

This is mainly how it’s used today, however corepack could add support for clients that talk to other registries (for example jsr) in the future.

In fact, this is already possible. See nodejs/corepack#354 and nodejs/corepack#359

@benjamingr
Copy link
Member

@styfle

This is mainly how it’s used today, however corepack could add support for clients that talk to other registries (for example jsr) in the future.

Sure, if/when the situation changes I will change my opinion based on those new facts. Currently we have only unofficial npm clients on corepack (and optionally the official cli).

I feel corepack does two things:

  • Manage versions across package managers across projects (like volta).
  • Install other package managers.

Honestly if the pain is the former - we can let people opt in for a package manager the first time they use it in corepack (instead of installing it ourselves) making it explicit it's third party software not authored by or vendored with node. Package managers would opt in (yarn being installed in corepack instead of corepack installing yarn for example).

Then we let corepack manage versions across projects of package managers (yarn, pnpm, jsr, whomever) but remove any risk of liability or dependence/endorsement. It would require a tiny bit more code on the package managers' side but would probably be a good compromise.

Alternatively making corepack an npm module itself rather than shipping it in core also has similar benefits but I assume that would have slightly worse dx hence the opposition some people have for it.

@acopier
Copy link

acopier commented Aug 18, 2024

Users needing to use npm to download their favourite package manager feels unfair to me while npm is bundled with NodeJS.

@RDIL
Copy link

RDIL commented Aug 18, 2024

Very disappointed to see how the ecosystem is moving backwards in real time with this PR. The only purpose this serves is to increase the friction of using other package managers. I think the user reactions in the OP serve as a clear indication as to how Node users feel about this change.

@ovflowd
Copy link
Member

ovflowd commented Aug 18, 2024

Since this PR is getting a lot of traction on social media, YouTube videos, and blog posts. I'm locking this PR for collaborators only to prevent this place from becoming an off-topic nightmare.

We hear you, the community, and we are already aware of your dissatisfaction with this change and how it may be perceived. There is an active discussion in the Package Maintenance Working Group to come up with a plan that could work for everyone.

If you're interested in more details about the following steps, the plan, and why it is possibly being done, feel free to engage here or on the OpenJS Slack.

Feedback from the community is always welcome, and it is a priority of this project. Unfortunately, this PR is not exactly the right place for it.

@nodejs nodejs locked as off-topic and limited conversation to collaborators Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"packageManager" field doesn’t support npm