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

doc: state that removing npm is a non-goal #51951

Merged
merged 8 commits into from
Mar 15, 2024

Conversation

GeoffreyBooth
Copy link
Member

As part of resolving the request of the members of the TSC who met on 2024-01-24, this PR aims to help clarify the goals of Corepack. In particular, this clarifies that it is a non-goal of Node.js overall, and therefore Coreback by inclusion, to work toward removing npm from the Node.js distribution.

This also codifies what was written in #50963 (comment), which seems to be a consensus:

  1. we solidify the “special relationship” with the npm client, clearly stating that it’s vendored because it is the reference implementation for the npm registry, as well for historical reasons.

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

@GeoffreyBooth GeoffreyBooth added doc Issues and PRs related to the documentations. npm Issues and PRs related to the npm client dependency or the npm registry. labels Mar 3, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@GeoffreyBooth GeoffreyBooth added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 3, 2024
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

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.

I'm -1. Unbundling npm should be a goal for the long term.

@mcollina
Copy link
Member

mcollina commented Mar 3, 2024

I think it's good we have a vote on this. cc @nodejs/tsc

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 4, 2024

I'd like to pose a much higher level technical question before we get too into the weeds of "what package manager ships with node"

Here's the question

"Should node.js ship with a package manager".

I've heard a lot of back and forth, and various takes that seem far more focused on "politics" or "fairness" without answering a code technical question about an architectural decision.

I would argue ABSOLUTELY. Unless Node.js wants to fundamentally shift the way it distributes, such as shipping a toolkit like rustup or a higher level version manager (something that was previously discussed and I think could be amazing for the ecosystem) it makes no sense to ship without the source code for a package manager so that we are shipping a complete and useful tool out of the box. I can go on about this at length if someone truly wants to debate this, but synchronously and dynamically fetching a critical tool would make every single ci/cd flow slower (which is also a reason I personally prefer just shipping the source of yarn / pnpm in core... Because having to bootstrap a package manager for every build job from various sources is just slower).

Edit : adding context for clarity to below paragraph

In response to recent comments, a call to vote, and finding resolution to this PR, I would take it a step further and argue that unbundling npm, as a standalone goal, is not acting in the best interest of the project... And as such it should not be a documented goal. If a new solution that genuinely makes Node.js better for all development use cases results in removing npm from the source tree you will not see me protest. Claiming removal of npm as a goal or non-goal imho misses the point. We should focus on solutions.

I'd love to see a goal like "improve overall getting started developer experience for new and long time developers alike" or "make container based build pipeline more efficient for better cold starts".

In all of the talk of fairness I haven't actually heard anyone point out a way that unbundling of npm makes the lives of Node.js developers any better. I have heard compelling evidence of how corepack makes the lives of yarn + pnpm developers better... Which is why I have advocated for it's inclusion in spite of my technical concerns with the project.

I think we could all benefit from taking a step back, asking some basic questions to help frame the important decisions being made, and make those decisions within that framework.

  1. Does node.js desire to unbundle all package managers and only fetch package managers dynamically?
  2. If fetching package managers dynamically do we want to have a blessed way to do so?
  3. Are we ok shipping multiple package managers?
  4. Can each package manager have a say in the architecture of how they are embedded and distributed in nodejs?
  5. Do all package managers need to support a baseline set of features to ship with node.js?
  6. What are the requirements of a package manager to ship with Node.js (this one is critical and I think extremely important to answer before landing anything to avoid endless future debate)

I'd actually really appreciate it, personally, if we could step back from some of the rhetoric and posturing and try and build some consensus around the architectural decision related to the question above. It really feels like we are poised to make critical decisions with ecosystem level impact without having a clear decision making framework and that is really disappointing.

If possible, I'd also really like to stop the debate around unbundling npm or not. I recognize my bias and conflict of interest here. To be clear, I think making the statement on its own is problematic as it over simplifies a complex problem space. Let's be clear about the goal and why. Simply unbundling npm makes Node.js worse without a clear solution in place to replace it. If the goal is to "only dynamically fetch package managers" or "distribute all package managers via corepack" it feels a lot less targeted. Simply saying "our goal is to unbundle npm" comes across extremely hostile to a team of developers who do a lot to support the Node.js project and JavaScript ecosystem, independent of corporate affiliation. If your goal is instead "only independently or neutrally maintained package managers" call that out, up until this point nothing of the sort has explicitly been said. Let's have goals and solutions not problems and othering.

@GeoffreyBooth
Copy link
Member Author

Related: pnpm depends on npm.

@aduh95
Copy link
Contributor

aduh95 commented Mar 4, 2024

I would take it a step further and argue that unbundling npm, as a standalone goal, is not acting in the best interest of the project... And as such it should not be a documented goal. If a new solution that genuinely makes Node.js better for all development use cases results in removing npm from the source tree you will not see me protest. Claiming removal of npm as a goal or non-goal imho misses the point. We should focus on solutions.

+💯 on that, that's also my point

Also I'd like to state that the idea of "refining node goals" would help refine Corepack goal simply doesn't add up; things can have conflicting goals and still work together. Case in point: Node.js collaborators have sometimes conflicting goals, yet we're able to work together. (and also, Corepack goals should be irrelevant, it's a tool, it cannot act on its goal; it has functions, which improves/worsens UX, we should focus on that)

@richardlau
Copy link
Member

I think this could be rephrased in a way that isn't specific to npm or the whole package manager debate.

  1. A goal of the project should be to minimize disruption to the ecosystem. Which is not to say we can't ever make a breaking change, but that it should be conscious and considered.
  2. Once something is added to Node.js, and is not experimental, the bar for removal is high, referring back to 1.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Mar 4, 2024

Also I’d like to state that the idea of “refining node goals” would help refine Corepack goal simply doesn’t add up

I think it matters a lot what we’re trying to achieve, because that’s the only way to evaluate whether something is the best solution to the problem that we want to solve. For example, if the problem we aim to solve is that we want Node to ship a built-in package manager version manager, then sure, Corepack is a solution and possibly the best one. (An alternative is for us to bundle asdf.) If the problem we want to solve is that we want to provide access to Yarn and pnpm without requiring the user to install them first, then Corepack is a much larger solution than is necessary (as compared to #51931, for example). If the problem is that we want to stop bundling npm or any other package manager, well, I find it hard to see how Corepack solves that unless we plan on ignoring and overruling the npm team’s objections to npm being managed by Corepack, or Corepack being redesigned substantially. We can’t even begin to have these evaluations without agreement on what problems we’re trying to solve, though, and then deciding whether the best solution for those problems is Corepack, some alternative, or some version of Corepack that has design changes.

The purpose of this PR isn’t to zero in on what problems we are trying to solve; I’m first trying to rule out one problem that we’re not trying to solve, to gradually narrow our focus. Assuming this PR lands, then yes @richardlau, the next step would be to try to further define what our goals are, in a positive way. But currently we’re all over the map in that regard so I think excluding this one from the mix as a first step would help focus people on the remaining options, hopefully one or more of which might get majority support.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Marking my explicit disagreement with the concept of defining “goals” for the project. We should follow our own internal goals, reject PRs that go against those goals, approve PRs that align with those goals, and ignore PRs that are orthogonal. Trying to agree on a set of goals is an endless work that is simply not necessary for running an open-source project. Also, if Node.js defines set of goals for the project, I don’t intend to follow them or enforce them, and instead keep following mine and be respectful that others may have different goals.

@GeoffreyBooth
Copy link
Member Author

Marking my explicit disagreement with the concept of defining “goals” for the project.

We already have https://github.com/nodejs/node/blob/main/doc/contributing/technical-priorities.md. If you want, I can say we’re defining “priorities” rather than goals to match the language, but I don’t find that a distinction that matters. I don’t think it should be a “priority” for the project to unbundle npm.

@GeoffreyBooth
Copy link
Member Author

@aduh95 @richardlau @wesleytodd I’ve rephrased to avoid the previous “non-goal” wording. Please take a look.

@richardlau I was going to add language about the prioritization of stability but it’s already here: https://github.com/nodejs/node/blob/main/doc/contributing/technical-values.md#2—stability. I think we need a section specific to package managers because the current text isn’t explicit enough to make clear the project’s position regarding whether or not npm should be included in our distribution, and whether or not other package managers should be included. I think the new text is explicit enough to make that clear. We already explicitly call out other technologies, such as TypeScript, so I don’t think that this is out of place.

@arcanis
Copy link
Contributor

arcanis commented Mar 4, 2024

and whether or not other package managers should be included. I think the new text is explicit enough to make that clear

The new text is incorrect since Corepack has already been voted in by the TSC, which thus already decided that the project shouldn't ship exclusively with npm.

As a result, that Node.js only ships with npm isn't "in accordance with our distribution policy", as that would imply that adding other package managers through Corepack would contradict this policy.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@anonrig anonrig added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 15, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 15, 2024
@nodejs-github-bot nodejs-github-bot merged commit 8824adb into nodejs:main Mar 15, 2024
18 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8824adb

@GeoffreyBooth GeoffreyBooth deleted the npm-removal-non-goal branch March 15, 2024 01:14
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#51951
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luke Karrys <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
PR-URL: #51951
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luke Karrys <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
PR-URL: #51951
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luke Karrys <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
PR-URL: nodejs#51951
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luke Karrys <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. npm Issues and PRs related to the npm client dependency or the npm registry. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.