-
Notifications
You must be signed in to change notification settings - Fork 29.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
deps: Experiments with Corepack #35398
Conversation
Fantastic to see progress on this! With regards to the questions:
I'd say that's a requirement, yes.
To do so, we'd need an issue opened in the nodejs/admin repo proposing the change. Assuming there are no objections after a few days, it's considered accepted and we just move it in. There's no real fanfare here.
Take a look at the test/parallel/test-npm-* tests and follow a similar pattern. The testing does not have to be comprehensive but should be enough to be reasonably certain that changes in core haven't broken something in corepack. |
@@ -0,0 +1,2 @@ | |||
#!/usr/bin/env node |
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.
these files could be symlinks to a helper that reads process.execName or whatever it's called. it's a common pattern in gnu core utils so it shouldn't break anything.
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 also occurs to me that if someone does not want a specific package manager on their system (for example, i do not wish to have yarn on my system), this could be considered somewhat invasive. Are we planning to ship tars with and without corepack?
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.
@devsnek ... this would not install the binaries by default. It installs jumpers that would download the binaries on demand if needed and it would be possible to block it via configuration.
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 realize it doesn't come with the binaries, but it puts things in my path that installs them. Let's say I'd rather not have any of that. Would I have to bundle my own releases of node?
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.
What if it will ask you before downloading Yarn or pnpm?
Like "Do you want Yarn v2 be downloaded to your system? Y/n"
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 not sure if it's implemented yet or not, but there was also discussion around being able to set configuration options to disable the download. It would likely be possible to have the tool remove the jumpers from the path when requested to do so. For interactive installers, it should be possible to make that configurable (e.g. by default install all jumpers but give the user the option to select them during the install)
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.
Downloads can be disabled by setting COREPACK_ENABLE_NETWORK=0
in the environment (nodejs/corepack#4). Removing the jumpers isn't possible yet as I wasn't sure of the use case, although it would be possible if needed.
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.
@merceyz many tools are invoked directly, and not as part of a package manager - how would these tools detect it?
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 it should be up to these projects to make decisions on a per-case basis, as the semantic compatible path isn't clear. For instance, that CRA "will use Yarn to install dependencies (when available)" could be interpreted as CRA preferring installs to be made via Yarn as much as possible - in which case using npm when Yarn is available wouldn't be the behavior that the project would prefer.
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.
Also note that these tools are typically called via npx
, pnpx
, or yarn create
/ yarn dlx
. In all of these cases, the tool got an implicit insight as to the preferred package manager (ie under npx
the $npm_config_user_agent
value will reference npm, with yarn dlx
it'll reference Yarn, etc).
There still seem to be some lingering TODOs from the issues linked in the original point before this should land:
I'd also like to ask:
Additionally, I think it's worth being upfront about our intents here: do we plan to eventually try to install npm through this? This should be probably be discussed since it could be impactful in deciding how to approach landing this / if it should be landed. Others shared their views in some of the aforementioned issues, and I have my own opinions on this that I'd be more than happy to share if they are potentially relevant. |
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 objection to land explicitly until we:
- move the project to the org
- (optional) onboard more maintainers (as per @bnb comment)
- make sure this ships as experimental somehow (so it can be included in v15)
- I don't think a doc-only experimental notice is enough, this should probably be an opt-in feature on v15
All tier 1 and tier 2 platforms listed on https://github.com/nodejs/node/blob/master/BUILDING.md#platform-list must be supported before this moves out of experimental (imo it's fine to land without initial support for some platforms if this feature is experimental and opt-in) |
To some of @bnb's questions:
I do not believe that these need to be answered before this lands and while it is still marked experimental. I would agree that these are exit criteria to graduate from experimental, however.
We really won't know until it's in user's hands, for that, we should land it as opt-in experimental.
They should be able to, yes.
We haven't applied that strict of reasoning to other things and we shouldn't now. There's no guarantee that anything we add to Node.js is going to have people willing and able "without question" to support it for the next decade. That said, this is the whole reason for wanting to bring the project into Node.js and encourage additional maintainers. That said, this thing is really small and shouldn't need much effort to maintain.
Given that it's an install-time thing and not a Node.js runtime thing, the "flag" would be an option in the interactive installer to omit it, which absolutely should be the case. |
I think there will be no lack of maintainers. This feature would be really useful for pnpm users, so I'll help out if there'll be a need. I believe other pnpm users/contributors will be willing to help as well. |
I have some concerns.
|
Well, to be clear, corepack does not currently replace the npm binary. So there's no immediate risk there. That said, the
Pull request... No different really than updating the npm client version in core.
People who do not want yarn installed should not ever need to have it. Shipping all three would bloat the installer image needlessly, and providing an option to download on install is really no different. This gives the additional benefit of allowing different projects to select which package manager they require, which an install time only option would not provide.
Not sure what you're basing this on. The design documentation for corepack goes into detail on how both corepack and the package manager would be updated and the flow is not much more complicated than what we have currently. |
@jasnell |
I don't think anyone suggested how to make this opt in for v15 yet, and imo if it's opt out it's definitely a semver major change (especially with @ljharb point on |
As @jasnell mentioned the npm binary is untouched by this PR, and I personally don't intend to change that, as I don't feel it's my place to make any critical change about a competing project (plus it comes with its own set of considerations unique to its situation). My only goal here is to ensure that my users' interests are accounted for - nothing more, nothing less 🙂
The LKG releases bundled with Node will be updated by pull request (just like npm is updated by pull request). The LKG releases that apply on an individual user's machine (typically what people would change by running Note that
(note: pmm is now Corepack, since pmm was already taken on the npm registry) My main worry with making it opt-in is classic: if there's a problem, we will be less likely to know about it - meaning that once we reach the end of the experiment, even if nothing bad happened, we still won't have enough confidence that the workflow will be stable enough for an even release (especially since Node wouldn't monitor how many people opted-in, so we would have to assume that it would be a very low amount). By contrast, seeing real-life feedback for an opt-out would guarantee that nothing sneaked by us, allowing us to safely make informed decision later down the road. And of course that wouldn't prevent us from having |
On this point there is a difference between shipping the package managers in the installer image vs option to download on install in that the former allows installation on a machine without an internet connection. |
@arcanis, based on the feedback, the concrete changes I'd like to see before moving this forward:
Given that this does not impact npm at all, there really shouldn't be anything further to discuss there unless the npm team decides to adopt it. |
also |
I still don't think we can switch from opt-in to opt-out in a semver minor (not as it is today at least). IMO this is not something we should rush as it has the potential of breaking many automated workflows via scenarios we didn't consider so far. |
For give me if I am wrong, just trying to understand things, first I love the idea of a level playing ground for all package managers, but why not just have If I use npm for everything on homebrew. Ican just do I am worried about the complexity and UX of having all these extra commands and things going around. I know myself using this would be trivial, but my co-worker who updates our gatsby site and only knows what commands we teach her and often messages me because |
That would more or less be something that |
This message might answer your question: #15244 (comment) Which package manager to use is not up to the user but rather up to the project. All developers that work on a given project should use the same package manager. |
Ok, so if corepack doesn't install a I'm not sure how this needle is being threaded here. Either there's an executable in the |
Unfortunately approach doesn't fit any Linux installed that I'm aware of (and we don't maintain the
The idea is that no extra commands will be required (you need to run |
npm is still packaged in its entirety, not part of corepack. |
@isaacs there will be no changes to npm at this time. If a user runs npm, npm will run, that's it. |
Ok, so the idea is that node ships npm and corepack, with a What happens when someone runs If that's the case, then npm can self-update and yarn can't (and can't be installed by npm either). Not exactly a level playing field. |
|
I'm basing it on the fact that
If you are shipping a binary named
It would increase bytes, but reduce complexity. I believe the tradeoff is better. Download time is less valuable than debugging time.
Should this just be a thing npm can do? It wouldn't be hard at all to add an npm command that will run a command with whichever package manager is declared in the project, and/or raise warnings/errors if you've specified a package manager other than npm, with instructions on how to install the package manager specified. |
The intent here is to change how both yarn and pnpm are installed and updated so this point is a bit moot. That is, |
Co-authored-by: Antoine du Hamel <[email protected]>
This comment has been minimized.
This comment has been minimized.
I think we are getting pretty close to the summit so I'd ask that we hold off landing this until the summit or alternatively if waiting for that does not make sense, add to the TSC agenda for the next meeting for a discussion there. |
@mhdawson I mean, five of them were entirely dismissed as irrelevant for inclusion if we're shipping this as an experimental feature. I disagree with that dismissal because IMO this is a situation where once we ship this we'll never be able to back off of it and should therefore have the context defined before shipping, but I don't think I'll be able to meaningfully convince anyone of that. |
@bnb I share the concern about once in as Experimental it will be difficult to back out. I don't think we should be dismissing questions on that basis. The answer to my earlier questions were that issues had been addressed and I was not entirely sure of that and it sounds like you are confirming that it is not the case. I'm hoping that in the discussion at the summit we can surface remaining concerns and get broader consensus on how to move forward. |
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.
+1 on waiting for the summit (marking explicitly as request changes so we don't land by accident)
Any news about it? 👍 |
In terms of "The corepack binary is included by default but not yarn / pnpm, so users have to opt-in via corepack enable" To confirm this this means that unless I opt-in via corepack enable, running yarn will result in a "not found". I'm very uncomfortable with "silently downloading" binaries without an opt-in by the user. |
@arcanis This needs a rebase to resolve the git conflict. |
@aduh95 I was thinking about reopening a fresh PR with the recent updates in the Corepack repo, along with a few other I plan to do (like the rebase you mention), is that ok? Discussions in this thread have mostly be meta, I think it would benefit from a reset focusing on its implementation. |
Done - I've opened #39608 which updated Corepack and rebased it against the trunk. Archiving this PR! 🙂 |
Context (note that pmm got renamed as Corepack in order to be an available package name on the npm registry):
This PR adds Corepack into the Node distribution pipeline (only for Linux / OSX - I didn't find how the Windows build works). By itself, it only involves very few changes:
deps/corepack
(it only takes 300KB)deps/npm
, this folder is mirrored intolib/node_modules/corepack
bin
(using the same pipeline as npm)make corepack-update
) updates thedeps/corepack
content to match the repository main branchAs a result:
npm install -g yarn
, which will overwrite the Corepack binary starting from 1.22.10Questions:
Documentation:
I haven't written them yet, but I suspect I'll need to update the following pages:
Do you see additional locations I should be mindful of?
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes