| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Review requested:
|
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
|
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
I don’t think so. Corepack is a package manager version manager. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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:
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
PR-URL: #51918 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
PR-URL: #51918 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
| Back | FazBrowse Home | New Git URL |
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