-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
base: main
Are you sure you want to change the base?
deps: remove corepack #51981
Conversation
Review requested:
|
fe4e060
to
34916d2
Compare
34916d2
to
38b97a8
Compare
I mean if we are not removing npm then this is kind of a valid point... |
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.
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? |
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. |
One command in a bash script/Dockerfile/instruction is too hard/burdensome for many users.
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. |
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.
But they can't pin versions. I'm -1.
You can pin versions. |
So these users are sophisticated enough to use a package manager but too novice to install a package manager? |
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":
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. |
Not applicable anymore. See #51981 (comment) |
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. |
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.
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.
After a careful thought I changed my mind. LGTM. |
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 You can install a package manager npm i -g yarn You can pin a package manager version to your project. npm i yarn@{version} |
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.
I’m -1. corepack is part of the flow of many devs.
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.
lgtm
I changed my mind due to nodejs/corepack#495.
Corepack poses a significant security risk as it is.
@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 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 |
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. |
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. |
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.
Changed my mind
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.
I no longer work for Microsoft and believe it's fine for me to take a position on this (officially).
I still think we shouldn't remove corepack |
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 Also could minimize the negative imact |
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 |
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. |
I've been using pnpm exclusively through corepack. This PR introduces unnecessary complexity to any setup using corepack for no reason. If anything, corepack should be bundled by default instead of npm. |
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. |
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. |
nodejs/package-maintenance#606 (comment) Our meeting is in ~30min, please come and discuss. |
Agreed with all statements. This allows us to better take care of Corepack without worrying about the implications of it being within Node core. |
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 |
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:
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. |
Users needing to use npm to download their favourite package manager feels unfair to me while npm is bundled with NodeJS. |
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. |
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. |
"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
ornpx corepack
) or source as they were prior to #39608.Fixes: #51888
Related to: #51951
@nodejs/tsc @nodejs/corepack @nodejs/npm @nodejs/package-maintenance