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: add policy for distribution #51918

Merged
merged 8 commits into from
Mar 3, 2024

Conversation

GeoffreyBooth
Copy link
Member

This is my attempt at writing down what seems to be a rough consensus emerging in the various Corepack-related threads such as #50963. The purpose of this PR is to confirm that what’s written here is a consensus or majority opinion (if it comes to a vote) to help focus the decision-making around Corepack. This file can always change (or be removed) in the future; all it takes is another PR.

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

@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 Feb 29, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs nodejs deleted a comment from wesleytodd Feb 29, 2024
@GeoffreyBooth
Copy link
Member Author

I rewrote based on overall feedback. I took out the non-goal and kept things positive, and put the “for historical reasons” stuff in the introduction rather than making it about npm specifically.

Obviously we have more external projects included than just V8 and npm, but I think we can start here and add to this list in follow-ups; I’m more interested in seeing if we can reach consensus on the policy itself first. I don’t think we need to list projects like eslint or Undici that aren’t exposed to users.

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

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

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

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

As a note, this policy being accepted has one direct impact:

It necessitates removing corepack from the node distribution. Corepack is a package manager and thus is duplicative of npm. I am 👍 on this, but want to make sure folks are clear on the impact of this policy.

@GeoffreyBooth
Copy link
Member Author

It necessitates removing corepack from the node distribution. Corepack is a package manager and thus is duplicative of npm. I am 👍 on this, but want to make sure folks are clear on the impact of this policy.

I don’t think so. Corepack is a package manager version manager.

doc/contributing/distribution.md Outdated Show resolved Hide resolved
doc/contributing/distribution.md Outdated Show resolved Hide resolved
doc/contributing/distribution.md Show resolved Hide resolved
doc/contributing/distribution.md Outdated Show resolved Hide resolved
@wesleytodd
Copy link
Member

wesleytodd commented Mar 1, 2024

I don’t think so. Corepack is a package manager version manager.

I don't think this is a meaningful distinction. Package managers manage package versions as well. If we think this policy sill has room for that interpretation than I think it is not clear enough and will retract my approval.

@aduh95
Copy link
Contributor

aduh95 commented Mar 1, 2024

We should probably clarify whether landing that policy means we have to remove node:http or Undici, as two competing HTTP client implementation. Same thing for WebCrypto and node:crypto.

@wesleytodd
Copy link
Member

wesleytodd commented Mar 1, 2024

We should probably clarify whether landing that policy means we have to remove node:http or Undici, as two competing HTTP client implementation.

Those two do not do the same thing. node:http does not implement fetch. And that overlap is a part of why the entirety of undici was not exposed.

@wesleytodd
Copy link
Member

Maybe we need to include wording to say that "evolving web standards and other considerations might mean the project bundles software with overlap of exiting api" or something? This policy is intended only to cover vendorerd and distributed parts, so maybe that is not clear enough yet as well?

@aduh95
Copy link
Contributor

aduh95 commented Mar 1, 2024

The same argument can be made for Corepack and npm: npm does not implement support for "packageManager" field, they do not do the same thing. In any case, having clarifications is a net positive, even if not all of us think they all are necessary.

@wesleytodd
Copy link
Member

having clarifications is a net positive, even if not all of us think they all are necessary.

Yeah this is the goal. I would rather not derail that goal on too many details, but if formally defining the distinction between npm and corepack is required to get that clarity then I think we may need to have that discussion. Do you believe that The same argument can be made for Corepack and npm is necessary to dig into to achieve that goal?

@aduh95
Copy link
Contributor

aduh95 commented Mar 1, 2024

My point was, in the same way it feels obvious (to you at least) why node:http and Undici are not the same thing, it also feels (to me at least) obvious that Corepack and npm are not the same thing; in each case, both have very different set of features, with little overlap.

@GeoffreyBooth
Copy link
Member Author

it also feels (to me at least) obvious that Corepack and npm are not the same thing; in each case, both have very different set of features, with little overlap.

npm can install dependencies like lodash, whereas Corepack only installs package managers like Yarn and pnpm. npm only updates dependencies when you ask it to, whereas Corepack changes package manager versions with every command. They’re very different.

@wesleytodd
Copy link
Member

My point was, in the same way it feels obvious (to you at least) why node:http and Undici are not the same thing, it also feels (to me at least) obvious that Corepack and npm are not the same thing; in each case, both have very different set of features, with little overlap.

Ah interesting. So one thing I strongly believe is there is a solution to the other things corepack is doing which I would be fully in support of (something that includes the scope talked about in the pkg metadata proposal, and inclusive of more than just package managers). The problem to me with corepack specifically is that it does not do those things, and seems to not be open to the discussion of adding them.

I see what you mean though, and totally respect your differing and valid opinion. I guess the question then is, how can we update the proposal doc to drive toward agreement on what the scope of each is and where we would want to define the line between "the same" and not.

@wesleytodd
Copy link
Member

wesleytodd commented Mar 1, 2024

npm can install dependencies like lodash, whereas Corepack only installs package managers like Yarn and pnpm. npm only updates dependencies when you ask it to, whereas Corepack changes package manager versions with every command. They’re very different.

I dont think this is a meaningful set of differences. I can configure npm to install latest or an ever changing dist-tag. I could disable lock files and then setup my project to have a yarn dev dep like that and run npx yarn. Like all of these options achieve the same goal of users not having to think about them in individual projects. If we land the proposal in the other repo for npm to support a unified format for devEngines then it would even fail if folks accidentally used the default shipped with node, preventing one of the main user complaints.

EDIT: and all of these things are done with existing tools and security risk/surface area not necessitating a new tool, new fields, new patterns in the ecosystem, etc.

@GeoffreyBooth
Copy link
Member Author

Even if you could kind of contort npm to do some of the things that Corepack does, it doesn’t do all of them, and not in the same way. It’s certainly not designed to intercept Yarn or pnpm commands and ensure the correct version of Yarn or pnpm is running before allowing the command to proceed. The two tools have very different purposes, even if some functionality overlaps between the two.

If you want to argue that Node simply shouldn’t be in the business of shipping a package manager version manager, that’s fine, but that’s out of scope for this PR. My goal here is to document the current state, not to change it. Once this PR lands you could open a follow-up to suggest changing things.

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2024
@wesleytodd
Copy link
Member

If you want to argue that Node simply shouldn’t be in the business of shipping a package manager version manager

All existing package managers in this ecosystem are packages themselves, so I still think I disagree on this distinction of "a package manager version manager", that is just "a package manager". Either way though, I think your other points are really where this discussion has more value:

Even if you could kind of contort npm to do some of the things that Corepack does, it doesn’t do all of them, and not in the same way. It’s certainly not designed to intercept Yarn or pnpm commands and ensure the correct version of Yarn or pnpm is running before allowing the command to proceed. The two tools have very different purposes

This I agree with. So if this is how we choose to define "the line between what overlaps npm and what doesn't" then I guess I will retract my statement that "this document necessitates removing corepack". I don't really agree with this definition, but I can see the points and so would not retract my pr approval over it.

And I think with this definition I would say "node should ship a binary version management tool", which is what corepack set out to do (just didn't execute well) AFAICT.

@GeoffreyBooth
Copy link
Member Author

So if this is how we choose to define “the line between what overlaps npm and what doesn’t” then I guess I will retract my statement that “this document necessitates removing corepack”.

This document isn’t meant to necessitate anything; I’m just trying to document current state. All that this document does is confirm that we’re all on the same page regarding what our current state is in terms of our policy for including external software in our distribution; and that any changes to that agreement would require a PR to this document, thereby ensuring that discussion occurs for any changes that deviate from this policy.

@wesleytodd
Copy link
Member

This document isn’t meant to necessitate anything;

I totally understand that. I think that the wording, if followed with my interpretation, does. I am alright if my interpretation is wrong (or just not what we want as a group) but if that is the case then we need to have more details about what npm is and what corepack is, and how that is different in the doc.

@GeoffreyBooth GeoffreyBooth 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 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2a70831 into nodejs:main Mar 3, 2024
18 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 2a70831

@GeoffreyBooth GeoffreyBooth deleted the package-manager-policy branch March 3, 2024 02:16
targos pushed a commit that referenced this pull request Mar 7, 2024
PR-URL: #51918
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@targos targos mentioned this pull request Mar 7, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51918
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51918
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#51918
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants