-
Notifications
You must be signed in to change notification settings - Fork 28.6k
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
build: enable yarn
and pnpm
Corepack binaries by default
#51886
base: main
Are you sure you want to change the base?
Conversation
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.
If we land this then we probably also need to add the additional shims to tools/msvs/msi/nodemsi/product.wxs. |
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.
Corepack needs to support npm in the same ways that it supports Yarn and pnpm, if Corepack is to remain included with Node’s distribution. While Corepack was experimental and disabled by default this incompatibility was something that could be ignored as a TODO to be solved later, but with the current proposal to enable it by default (for all intents and purposes, making Corepack stable) the time has come to reconcile its relationship with npm.
In particular, the members of the TSC who met on 2024-01-24 asked the Corepack champions to take specific steps to align design goals: #50963 (comment). This work has not yet been done. Until it has, I don’t think there should be any movement toward making Corepack stable or enabled by default.
I disagree. Having npm and corepack work together is an orthogonal concern. There is no impact to npm users if corepack is enabled by default. Recognizing that there still may be work remaining to do, these things can be done incrementally I don't see them as blockers here. |
If Corepack is to ever support npm in the ways that npm’s maintainers desire, then Corepack itself will likely need to undergo significant changes. It may need to drop the version pinning or jumper binaries, for example, which would be a dramatic change that would break the majority of Corepack’s current users (pnpm users who are pinning precise versions to avoid corrupting lockfiles). I see enabling Corepack by default as equivalent to dropping an experimental flag. It’s a sign that we’re moving an experimental feature into the “release candidate” stage, where we don’t anticipate any further major API changes. But unless we’re okay with simply never fully supporting npm, Corepack needs significant changes. And shipping major changes to a feature that doesn’t require opt-in is bad UX. We should sort out these things while Corepack is still experimental and still disabled by default. |
From what I've read on Github and from private discussions, NPM does not want to be shipped with Corepack. NPM wants to be shipped with Node.js itself. It was clearly communicated in previous Github issues. We shouldn't block the progress of adding support for other package managers just because NPM isn't interested, or it's not in their roadmap or it's not in their benefit. |
So maybe Corepack doesn’t need to ship package managers. Or maybe Corepack downloads Yarn and pnpm on demand but simply errors on the wrong version of npm. We don’t have defined goals of Corepack; what is a core feature of Corepack and what isn’t? If Corepack drops the downloading ability, would it still be achieving its goals? What if it drops the version pinning? My point is still that we should work all these things out before making Corepack near-stable. There’s still major uncertainty around Corepack. The fact that we’re considering enabling this by default before even making the effort to define what Corepack’s goals are feels very wrong to me. |
@aduh95 I know that a prompt was added in nodejs/corepack#360. What I'm not sure of is how "by default it's only shown when "not using the corepack binary" (i.e. when using the binaries created by corepack enable)" affects it's use in Node.js. I want to be sure that if corepack is enabled by default that the user will see the prompt the first time they try to use yarn, pnpm etc. Assuming that they will see the prompt, I think we need an addition somewhere in the Node.js documentation which explains that despite corepack being present to ease the installation of these tools, they are not part of the Node.js distribution and:
In addition we should have an addition to the https://github.com/nodejs/node/blob/main/SECURITY.md#the-nodejs-threat-model which indicates that any vulnerabilities in package managers installed through corepack are outside of the Node.js threat model and why. |
I’m currently -1, pending a solution to nodejs/corepack#399. I think this can be solved quickly if there is agreement. —— @GeoffreyBooth I think we can reconcile the conundrum by saying that npm will stay as it is, and it has a special relationship with Node.js. I agree that something in the corepack design docs has to change. |
Surely there should be consensus that we need updated design docs, both on the Node and Corepack sides, before we consider enabling Corepack by default. Even if the updated docs say that npm is special and nothing that Corepack does applies to it, that feels like something that we should spell out and land using the regular PR process. I don’t understand why we would be rushing toward stability when there’s still basic consensus-seeking work to be done. Personally I don’t think that npm should be special. If we ship both npm and Corepack, as a user I expect them to work well together and provide the same functionality as is provided for other package managers. Inconsistency is bad UX. My preference would be that the people working on Corepack devote some effort into resolving this so that Corepack can work well for all of our users, not just Yarn and pnpm users. |
Many changes may be needed by lots of things over time. We don't need to solve everything before making progress. We can make breaking changes in semver-major commits at any time. The idea that we might need to make major changes at some point is no reason not to make progress now. |
This argument makes no sense to me. Generally we don’t unflag features if we anticipate major changes. That’s what I see here: Corepack will need major changes to support npm; it should support npm; therefore it should remain disabled by default while it undergoes the significant churn required to support npm. The lack of outreach to the npm team even after requests from the TSC leads me to suspect that if Corepack is unflagged without npm support, then npm support will never happen. Therefore I think it’s all the more vital that we hold the line here until the design doc work and npm support work is complete. |
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.
Marking my -1 explicit depending on the fix in nodejs/corepack#399. The lack of reproducible builds by default is too high of a risk to mark as enabled by default. It's also a clear defense against supply chain attacks etc.
(I'm positive on lgtm'ing this PR once that is solved, I think this should happen ASAP. I'm currently flagging a major issue for the "newbie" use case).
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.
As I mentioned in the last TSC call in which we discussed this, I believe it's important to have an "Alternatives Considered" writeup in the Corepack docs and/or readme in which the team documents the reason why the Node.js runtime should ship Corepack by default and why it should be enabled by default vs alternatives such as using asdf, Volta, Docker, Corepack as a separate binary or any other environment wrapper/manager solution.
I reached out to the npm team. They are interested in finding a solution that can work for them as well as Yarn and pnpm: #51888 (comment) Such a solution would probably mean changing or replacing the I think we should let both efforts play out before we unflag Corepack. |
@GeoffreyBooth Couldn't npm just be another valid package manager in For example: "packageManager": "[email protected]" |
It could if the npm team would be willing to support that syntax, but they’re not. They want something that defines version ranges, not a particular version; what should happen when validation fails; and other concerns. We’ve started sketching out an alternative configuration block in openjs-foundation/package-metadata-interoperability-collab-space#15. |
One thing that was called out that still has not been clarified is
The current documentation simply says open a PR and it will land and it can become part of the default list after a couple of months. There is no documentation at all regarding how those package managers end up shipping in Node.js This doesn't seem like explicit enough governance for something that will result in jumper binaries being installed on every system that installs Node.js |
I don't foresee the
What do you suggest more than what is the current status for PRs to be merged in this repository? ie.
|
@@ -20,10 +20,14 @@ | |||
<String Id="NodeRuntime_Description" Value="Install the core [ProductName] runtime (node.exe)."/> | |||
|
|||
<String Id="npm_Title" Value="npm package manager"/> | |||
<String Id="npm_Description" Value="Install npm, the recommended package manager for [ProductName]."/> | |||
<String Id="npm_Description" Value="Install npm, a package manager for [ProductName]."/> |
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.
This change does not relate to enabling Corepack by default (ie. we can recommend npm
even though Corepack is being distributed). I'd suggest making a separate PR so we can discuss this change independently.
<String Id="npm_Description" Value="Install npm, a package manager for [ProductName]."/> | |
<String Id="npm_Description" Value="Install npm, the recommended package manager for [ProductName]."/> |
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 strongly urge folks, the @nodejs/tsc members specifically, to consider how contentious this issue is and go slow on a decision. There a group of folks working to propose a concrete way forward which addresses both the needs from the corepack
team and the other open feedback. I strongly believe that shipping this PR too soon is locking the ecosystem into supporting a subpar solution for a long time. I am not asking to unilaterally stop this, just not to vote on this too soon and with incomplete context. Also, I am not adding new technical context here, that has all been said, but I am appealing to your care for the ecosystem and the contributors who are deeply invested in this part of the ecosystem who are opposed to the shipping this proposal.
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 it is appropriate to enable corepack by default at this time. There are numerous concerns about it that have not been addressed; furthermore, it still breaks certain npm and pnpm patterns.
I'll perhaps be a little more direct than I should, but I very rarely felt as invisible as during this past week. If you attempted to have an outreach role, I'm afraid you failed at that: I personally feel you've ignored signals from my team, Zoltan had to reiterate his positions despite stating he didn't wish to be dragged into this discussion again, and the npm team represented by Myles repeatedly had to restate they were fine with Corepack as well. Or do you think this is unclear?
On the other hand we have you who decided to run a crusade noone in the three package manager teams understands, and gave yourself a mandate to propose an alternative that noone in the three package manager teams requested, or even supported. You say you want to treat us as valued stakeholders, and proceed to ignore our work. You talk about healthy relationships, and proceed to mischaracterize our words in the very same sentence. How is that coherent? I don't understand how this thread could derail so much without noone intervening, and it seems to me a significant miss from a governance and/or moderation point of view. |
After reading all the comments, and as a (relatively) outsider who hasn't been participating in these Corepack conversations, I feel that this PR is becoming unnecessarily emotional and throwing blows in directions that aren't called for or welcomed by our Community. I've read many discussions and threads of Corepack (recent and old ones) before deciding to write this commentary here: we are making a rushed decision on enabling Corepack. Listen, I do use Corepack and find it incredible. In most cases, it works out of the box, and as an average user who simply wants an efficient way of enabling PNPM or YARN without using third scripts, it is a great choice. Yet, I do acknowledge Darcy's, Geoffrey's, Wes's, and many other concerns (which actually do make a lot of sense, at least in my humble opinion) that Corepack, in its current state, seems at least unstable and has a few gaps on its documentation/governance (package manager adoption, version resolution, installation path, integrity, etc) These concerns raise the question: Should we enable Corepack by default now? Honestly, what is not working right now that would require us to proceed with this PR? As mentioned before, with Corepack disabled by default, you can still use it regularly, as it is enabled at the moment of invocation. And that's great since I only want to have Corepack install or download binaries upon my explicit request. I agree that the community is asking for better ways to use diverse package managers with Node.js, and I agree with Matteo's statement, "It doesn't need necessarily to be Corepack." That statement is vital because it reverberates with a sentiment I share: that we, as core collaborators, are here to serve the community. Yet, at the same time, to better serve the community, we need to make well-thought technical decisions correctly, and sometimes, we wait for specific pieces of technology to reach maturity or the so-called consensus-seeking process. I doubt that Darcy and other people objecting to this are doing it for the sake of being against Corepack, and it saddens me that some collaborators go to the lengths of pointing fingers and escalating the situation unnecessarily. Although we might differ on when Corepack should be enabled, we all agreed that we are seeking a long-term solution that solves the issue of "allowing Node.js to bundle or seamlessly provide package managers in a uniform approach. Be that Corepack or not. I genuinely ask the collaborators involved in this PR to take a small step back, relax, and figure things out together. I believe we can reach a consensus and work together to release a stable "Corepack." Having said that, I feel there's no immediate need for Corepack to become enabled by default. Again, it doesn't change the current status quo, but it forces the functionality into a "stable" status, which should honestly not be warranted at the moment. I understand that many people put a lot of effort here. Trust me, I definitely understand what it means to take a reasonable amount of time to craft something desirable. But if Corepack has been in the works for a few years already, addressing these concerns in a few extra months... Wouldn't hurt, would it? Peace 🤞 |
Hi @arcanis, could you please self-moderate? I understand you might feel frustrated at the moment, but this sort of commentary is not healthy. |
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 a member of the npm team and will be working on adding support for devEngines
to npm.
I think more time should be given to allow implementations of the devEngines
spec to be written. Progress has been made in the package metadata interop collab space and work on implementation is ready to begin. My goal here is to avoid any possible user confusion and I think marking Corepack as stable will have the effect of locking in a single solution to this problem while we are working on another.
As a Node.js collaborator, I also have concerns about a lack of documented governance and goals in Corepack.
@Ethan-Arrowood I'm going to need you to be more specific, I don't understand what would be the actionable steps to address your feedback. @lukekarrys @wesleytodd Requesting more time is certainly fine, note that this PR is labelled semver major, so it won't make it to a release of Node.js until the next major anyway. |
My apologies @arcanis that you didn't receive the notification about yesterday's meeting discussing the The meeting primarily discussed the |
@aduh95 I am echoing other's request for changes to ensure my opinion is similarly heard. Unfortunately, a simple When the group can reach consensus on the inclusion of corepack I am happy to approve necessary changes. |
This is quite a long thread now. Can some summarise the current |
I was asked this in the 2024-03-06 TSC meeting and I gave a list then, which is in the meeting notes: https://github.com/nodejs/TSC/blob/main/meetings/2024-03-06.md#nodejsnode. There have been objections raised since then, many of which are summarized in the top post of #52107. |
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 believe enabling corepack is what we all want to achieve. I am not having a full picture of what measures were taken to guarantee security concerns are fully addressed. I would love to have a short writeup about all measures including all concerns in concise language. For now, I can only abstain.
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 changed my mind due to nodejs/corepack#495.
Corepack poses a significant security risk as it is.
Fixes: #50963
Last version of Corepack has addressed (some of) the major concerns that was discussed in the TSC meeting last time:
IMO having Corepack enabled by default will overall be a clear win for Yarn and PNPM users (and won't affect npm users). Having it enabled by default does not stop those who don't like the "jumper binary" pattern to install Yarn and PNPM as they see fit.