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

Dynamic default versions #93

Closed
mcollina opened this issue Feb 17, 2022 · 7 comments · Fixed by #134
Closed

Dynamic default versions #93

mcollina opened this issue Feb 17, 2022 · 7 comments · Fixed by #134

Comments

@mcollina
Copy link
Member

One of the main reason we adopted corepack was to avoid work in nodejs/node to support multiple package managers. There is a measurable amount of traffic on our H1 due to vulnerabilities in the npm dependencies, and we are struggling to keep npm up to date. Unfortunately corepack increases this load and it makes it even less apparent and hard to track as the vulnerabilities would not be apparent to the users.

I propose that the config file include only the major version of each package manager. This will ensure that for a specific version of Node.js there won't be any breaking change (I consider the drop of a Node.js version a breaking change).
Then we will update the major version of each package manager whenever we ship a new major release of Node.js.

I would also recommend that the config file is not bundled in but loaded at runtime. This will simplify maintenance on the Node.js side so we could just update the config file without updating corepack (and viceversa). This also enables mulitple Node.js lines to have the same version of corepack but different defaults.

@arcanis
Copy link
Contributor

arcanis commented Feb 17, 2022

It could be interesting to make this a GH discussion, since there are multiple parallel discussion points.

There is a measurable amount of traffic on our H1 due to vulnerabilities in the npm dependencies

Out of curiosity, do you have stats on how many of those vulnerabilities were truly relevant, vs "rubber stamp upgrade" intended to silence a noisy report?

We are struggling to keep npm up to date

I'm really new to the Node infra and procedures so feel free to correct me, but isn't it something that should be "easy" to automate? What exactly makes this process difficult (I'd like to better understand that to take it into account in future design iterations)?

I propose that the config file include only the major version of each package manager. This will ensure that for a specific version of Node.js there won't be any breaking change.

This isn't guaranteed. Package managers can make mistakes too, and all upgrades have the potential to ship bugs that would break someone's workflow (as an example, I remember npm accidentally shipped a few years ago a rc which erased the home folder in certain circumstances). Even bug fixes can be breaking changes, depending on the perspective.


In the case of Yarn we don't really recommend people to use the global version anyway, so technically speaking it wouldn't affect us much; my concern more around how Node distributors would see the perspective of shipping non-deterministic code to their users (even if the package managers are currently lazily installed, they are still deterministic).

For example, should Corepack be enabled by default, do you believe you'd be able to convince Ubuntu to ship the Corepack shims with the logic you describe? Or would it effectively prevent Corepack from ever being enabled by default?

@arcanis
Copy link
Contributor

arcanis commented Feb 17, 2022

I would also recommend that the config file is not bundled in but loaded at runtime. This will simplify maintenance on the Node.js side so we could just update the config file without updating corepack (and viceversa). This also enables mulitple Node.js lines to have the same version of corepack but different defaults.

That seems reasonable, regardless of the discussion about dynamic versions 👍

@ljharb
Copy link
Member

ljharb commented Feb 17, 2022

That mistakes can be made doesn’t mean versions can be frozen; that’s what semver ranges are for - to make fixing those mistakes easy. It’s the same reason pinning dependencies in a package version is pretty universally considered a bad idea. It’s up to application users to use a lockfile and pin everything, and it’s best when nothing is pinned otherwise. I’d expect to be able to choose to lock down my package manager version, but by default I’d expect it to use a ^ semver range, like everything else.

@arcanis
Copy link
Contributor

arcanis commented Feb 17, 2022

I'm not really trying to convince you or anyone, mostly just to explain why this will be problematic to other people outside of the Node project (in particular system-level package registries which distribute Node and its binaries). I'm also not the one who needs to be convinced (I disagree, but if Node and its distributors are aligned then I have no objection).

Also keep in mind that system-level package registries may have their own official mechanisms to keep packages up-to-date, such as apt-get install unattended-upgrades for Debian. Would it be ok for Corepack to "auto-update" through a different mechanism? I have no idea, hence why this needs talks with the relevant maintainers imo.

Note that during such discussions, perhaps npx can be used as "prior work" of common JS tool that "auto-updates" and that's already generally available.

@ljharb
Copy link
Member

ljharb commented Feb 17, 2022

node isn’t officially distributed via apt or any other system registry; what they choose to do is irrelevant.

@mcollina
Copy link
Member Author

Out of curiosity, do you have stats on how many of those vulnerabilities were truly relevant, vs "rubber stamp upgrade" intended to silence a noisy report?

They are almost always rubber stamp upgrades. However for a significant part of corporate users a noisy report is problematic.

I'm really new to the Node infra and procedures so feel free to correct me, but isn't it something that should be "easy" to automate? What exactly makes this process difficult (I'd like to better understand that to take it into account in future design iterations)?

I don't think this is easy to automate due to restriction on CITGM and LTS checks. @BethGriggs could articulate this better than me. We would definitely welcome more automation.

This isn't guaranteed. Package managers can make mistakes too, and each upgrades has the potential to ship a bug that would break someone's workflow (as an example, I remember npm accidentally shipped a few years ago a rc which erased the home folder in certain circumstances). Even bug fixes can be breaking changes, depending on the perspective.

In this specific case, this would be up for the package manager maintainers. I'm ok with this being their responsibility.

my concern more around how Node distributors would see the perspective of shipping non-deterministic code to their users (even if the package managers are currently lazily installed, they are still deterministic).

We chat a bit about this during our TSC meeting and most TSC member impression was that corepack was not deterministic and that's why it was preferred compared to bundling the package managers directly. We had the impression that Corepack would have enabled us to ship multiple package managers without having the maintenance cost of keeping them up to date.

Unless it's dynamic, there is no difference between bundling corepack and yarn+pnpm directly.

@BethGriggs
Copy link
Member

We are struggling to keep npm up to date

I'm really new to the Node infra and procedures so feel free to correct me, but isn't it something that should be "easy" to automate? What exactly makes this process difficult (I'd like to better understand that to take it into account in future design iterations)?

I think the mechanics of updating npm in Node.js are fairly straightforward. We're mostly just reliant on npm cutting a new release with the required fixes (which can take some time). We do now have the npm-robot opening the update PRs (example nodejs/node#42039).

IMO the main struggle is shipping those npm updates in releases - particularly on the LTS release lines. The release process is thorough but also lengthy. It can take 4-5+ hours just to run through all the builds/tests/CITGM runs. We also consider our consumers, such as cloud providers, as shipping releases put a burden on them too. With that in mind, we intentionally aim for monthly releases for Active LTS (16) and less than monthly/ad hoc releases for Maintenance (12, 14).

Each time there's a new npm version we get pressured to upgrade on all relevant release lines, with added pressure in the cases where the old version of npm is showing audit warnings. The result is that we're pressured to expedite or do additional releases on the LTS lines just to keep npm up to date.

My concern with Corepack specifying specific default versions is that we're increasing that pressure - we'll get the same asks and pressures to update the defaults for pnpm and yarn too. On the other hand, I see Corepack with dynamic defaults as a great way to experiment and gauge the appetite for more independent Node.js and package manager versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants