`"packageManager"` field doesn’t support npm

GeoffreyBooth commented 8 months ago

Version

21.6.2

Platform

Linux a3bc0d85e2cf 6.6.12-linuxkit #1 SMP Thu Feb 8 06:36:34 UTC 2024 aarch64 GNU/Linux

Subsystem

Corepack

What steps will reproduce the bug?

docker pull node:latest
docker run -it --entrypoint bash node
mkdir app && cd app
npm init -y
node --eval 'const fs = require("fs"); const pjson = JSON.parse(fs.readFileSync("./package.json", "utf8")); pjson.packageManager = "npm@9"; fs.writeFileSync("./package.json", JSON.stringify(pjson, null, 2));'
npm install

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

I should get an error that my current version of npm (10.2.4) doesn’t match the version defined in the packageManager field (9).

What do you see instead?

npm 10 runs without complaint.

Additional information

I understand that npm isn’t a “supported package manager” per https://nodejs.org/api/corepack.html#supported-package-managers, which is linked from https://nodejs.org/api/packages.html#packagemanager; but npm is distributed with Node, so it should be a supported package manager. It is a bad user experience to ship two tools (npm and Corepack) that don’t work together.

Furthermore, I don’t want my version of npm to need to be pinned; I want to be able to specify a minimum, like “npm 10+” but not a maximum; or to be able to say npm@* to enforce that this project requires npm but no particular version. I don’t want the packageManager field to cause me to use a version of npm that may have security vulnerabilities that have been patched in a newer version of npm. The maintainers of npm recommend always using the latest npm version, and it feels wrong (and a poor security practice) for the packageManager field to contradict this.

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

darcyclarke commented 8 months ago

Based on your expected behavior, & as I have shared in various discussions, this particular goal of warning or erroring based on using an unexpected/undesirable version of a package manager is already accounted for when you define a corresponding engines value at the project-level. The packageManager field overlaps with this existing key/capability for npm (ie. engines.npm). You can do exactly what you want here & specify a node-semver compliant range (ex. { ... "engines": { "npm": ">10" } ... }) & you'll get the expected result (warning about a mismatch or erroring when --engine-strict is defined/configured).

engines.npm is a historical key/capability (introduced pre-yarn or pnpm - ref. https://web.archive.org/web/20150105222541/https://docs.npmjs.com/files/package.json#engines) & the newer package managers support their own variants (see screenshots/examples below) & will warn or error on mismatches. This fact seems to get lost in the Corepack/packageManager discussion because - as far as I can tell - many TSC members do not have experience with it &/or have historically leaned towards permissiveness in regards to engines support.

You will likely hear challenges that engines is not intended for use with "projects" but rather "packages", which is historically & factually incorrect (ie. it works for both). Notably, there's little-to-no difference in npm's documentation between the "project" & "package" contexts - they are often, unfortunately, interchangeable. I will concede that - today - I have not seen nuanced capabilities to validate engines only at the project-level vs. dependencies vs. either/or in a production or development context (although, in npm's case, any command that does not walk the dependency graph is - essentially - only checking the project-level engines definition). That said, this nuance is well within the package manager's wheelhouse to solve for (ex. via config/flags) without requiring net-new, top-level keys or redundant tools to accommodate.

A quick search (not all-encompassing) finds more than 1.3M references to engines definitions across GitHub today. I would challenge the value of node adding a new packageManager key to drive a similar outcome. The vast majority of developers are using engines (given this data) to codify their package manager expectations/support or are using a more all-encompassing tool to manage their global binaries & development environments (ex. containers) at the project-level specifically. It would honestly be more helpful to promote the existing key/definition & encourage package managers to improve the status-quo in regards to the nuance of context (both in capabilities & documentation).

Example project/`package.json` to show how `npm`, `pnpm` & `yarn@1` interpret `engines` definitions

GEOMcZ8WsAEtBEJ GEOMeETW4AAJqL3 GEOMfDPXEAA0JhH

GeoffreyBooth commented 8 months ago

A quick search (not all-encompassing) finds more than 1.3M references to engines definitions across GitHub today. I would challenge the value of node adding a new packageManager key to drive a similar outcome.

I suggest you open a new issue to propose removing packageManager, including the possible changes that would need to occur as a result (like would Corepack start using engines, or would Node just remove Corepack?).

darcyclarke commented 8 months ago

My position is that Node should just remove Corepack (& don't think I have anything new to add to that discussion, _{which continues to go in circles}). My comment here was just made to highlight why your expected outcome is already accommodated for/possible & for drive-by folks to also potentially learn something (apologize for it being long-winded).

GeoffreyBooth commented 8 months ago

Sure, removing the packageManager field and/or all of Corepack would solve this issue, of course. Someone can go ahead and make a PR to that effect and we can see what happens.

Alternatively the behaviors of Corepack can be altered to accommodate npm in the ways that its maintainers support:

No version pinning, or version ranges (probably should be the same as the engines field).
No automatic installation of specified version.
No jumper binary.

To put it another way, I expect npm to be one of the “supported package managers,” since Node ships with npm; and I expect a packageManager field to support npm in the ways that npm’s maintainers approve. So either npm is treated differently from other package managers (like the others get a jumper binary and npm doesn’t, etc.) or all package managers are treated the way npm is, assuming that the maintainers of pnpm and Yarn find that acceptable. If there’s no behavior for Corepack that works for everyone, then whatever works for npm needs to be the required behavior and the package managers that disagree can be dropped from Corepack support. As npm is distributed with Node, it is the required minimum that needs to be supported.

mcollina commented 8 months ago

note that it should not be the same as the engine field. packageManager is about what is used for development, not consumption.

wesleytodd commented 8 months ago

note that it should not be the same as the engine field. packageManager is about what is used for development, not consumption.

This was addressed partly in @darcyclarke's post above. The only situation not covered today by the engines field with current package managers is:

A library repo which uses a specific package manager (and version) at development time which is different than what it supports after publish. (EDIT: and this is a small enough scope we could solve it without an entirely new tool)

GeoffreyBooth commented 8 months ago

note that it should not be the same as the engine field.

I meant that the value should probably be the same, like in the engines example ">=0.10.3 <0.12". As opposed to creating a new syntax for this, as packageManager currently does.

Besides the developer/consumer distinction, the other thing that I don’t think engines currently supports is blocking people from using the wrong package manager. It may stop you from using the wrong version of npm, but it won’t error if you use Yarn or pnpm instead. That’s another nice feature of packageManager.

I think there absolutely is a way forward to preserve Corepack being bundled with Node and addressing the npm team’s concerns. Something like:

Rename packageManager to corepack, and make it take an object.
That object can have values like { "packageManager": "npm", "versions": ">= 10.0.0", "download" false, "strict": false } or { "packageManager": "pnpm", "versions": "=8.4.2", "download": true, "strict": true } that define the various aspects of how Corepack behaves: what package manager the project supports, what version(s) of that package manager, whether Corepack should download the package manager if not already available locally, and whether a failed validation should result in a warning or an error (like engines.strict). These values could have different constraints based on package manager; we could define "download": true as being unsupported for npm, for example.
We add some API to Node to evaluate the corepack field, like import { validateCorepack } from 'node:module' or whatever. Then the npm CLI tool can validate the corepack field in the same place it currently validates engines.npm, and behave the same way. This allows the npm binary to stay as it is now, without being replaced by Corepack.

I am disappointed that no one has put in the effort to try to reach a compromise whereby all the package manager teams are satisfied with how Corepack is designed, especially after the TSC asked specifically for a volunteer to do so. I understand the desire to get Corepack enabled by default as soon as possible, but I really don’t think we should skip over the potential for a much better product here that has buy in from the npm team as well as the Yarn and pnpm teams. Our users will be much better served by a Corepack that works well with npm and is approved by that team.

wraithgar commented 8 months ago

👋 I was pinged by @GeoffreyBooth to see if there was a path forward on this from the npm side. It's 7:30am for me and I'm only halfway through my first cup of coffee so I ask for a little grace here as I write an initial response.

From what I can tell, based on the discussion in this thread, and the devEngines PR, is that we are once again running up against the perennial node/npm problem of "it looks like we all forgot to think about how we treat our packages as dependencies sometimes, and as services other times". What I mean by that is if I run npm install lodash I am interacting with lodash as a dependency. This is wholly different than if I clone the lodash GitHub repo, and run npm install from there.

Currently engines does not distinguish between these use cases. The lifecycle events (i.e. install prepare do so but in a way that evolved over time as this reality was discovered, they were not originally designed with this distinction in mind.

ESM, to me, seems like another thing that is trying to address this. Though again it seems like it was done without actually being intentional about the distinction. Unless I missed something there is no "non module" type. The idea of not being a module isn't even brought up.

To that end, if we solve this (and I think we can solve this) I would like to see this distinction baked into the solution. devEngines sort of gets there, but in a way that still feels inelegant in that it's a single new top level package.json entry that isn't very declarative about the assumptions baked in.

Now for my opinions. These are pretty extemporaneous and do not represent any deep forethought or desire from the npm team. They are Gar opinions.

I really feel engines either needs to be rethought, or supported as a legacy field. The npm team itself has come up against issues where we wanted to declare a different set of operating parameters for developing a module than that of a consumer using it as a package. template-oss is a prime example. In fact that has a third limiting case that feels like scope creep at this point: The lifecycle scripts it sets up are only relevant in a single node version. But I get ahead of myself.

What we now thing of as "engines" is really "what version of node will this module work in, as a module" aka "production dependency node version requirements". I think that is probably the safest approximation of how it is being used today. We don't have anything that can say "this is the version of node that my devDependencies work in."

We also don't have a "package manager" analog to "engines". When engines.npm was created there were really no other popular node package managers. That field was de facto the "package manager" field. We now live in a world where yarn, pnpm, deno, and others have great package managers used by millions of folks.

So in conclusion for this initial train of thought: let's solve the problems intentionally. Let's solve the problem of specifying what package manager is supported during installation, versus development. Let's do the same for Node.js version.

This isn't going to be something we can solve in a week, but I do think enough people care about the problem and know the use cases they need to see supported that we can get somewhere that works for ... if not everyone then a critical mass of users.

ETA: I have more to add about the nature of this problem from a root cause perspective (i.e. incompatibilities between package managers) but have another appointment I have to attend to. I will comment those thoughts later.

wesleytodd commented 8 months ago

A library repo which uses a specific package manager (and version) at development time which is different than what it supports after publish.

And to address the gap mentioned above, here is an alternative proposal (discussed years ago and implemented yesterday) implemented with some discussion of the approach: https://github.com/npm/cli/pull/7253

EDIT: I had this drafted before @wraithgar's above post, sorry if it is confusing why I linked this after he already did. Stinking meetings make it hard to finish anything lol.

I think the problem pointed out above is that npm and team is rightfully opting out of an objectively worse solution for end users (jumper shims) than what was both already available (nearly, and completed with this PR) so it means the end users would be left with mixed messages on top of the projects which are supported by corepack doing so in a worse way than the alternatives.

This is, IMO a valid concern to be taken seriously. I know I was not present when corepack was original built and included in core, but if I had been I would have strongly opposed it in favor of the multitude of widely deployed alternatives (which seem to never have been discussed publicly). So while it is hard to see these continued and sometimes confusing discussions, I think it is important that the project does try to do what is best for the users and ecosystem.

GeoffreyBooth commented 8 months ago

I think these would be the broad strokes of a cross-compatible solution:

We write up a document somewhere that specifies a new place in package.json that defines a few things:
- The project’s desired package manager(s)
- The allowed range(s)
- What to do when the current package manager fails validation (warn, fail, download and run, etc)
- If download, what URL to download from
- If download, the hash to validate the download
Corepack reads this new configuration instead of its current packageManager field.
The npm CLI also reads this new configuration and acts on it the way it currently supports the engines field. I assume npm wouldn’t support the “download” option, so it could just error and exit if that’s what’s specified.

This would allow npm to stay out of the jumper binary approach that they disagree with, while still providing the validation that Corepack does for Yarn and pnpm. From the user’s perspective, all three package managers support this new configuration for defining what package manager to use for developing a project.

So the next step would be to draft such a document and get all sides to agree on it. Does this work for you, @wraithgar?

wraithgar commented 8 months ago

Perhaps it may be useful to come up with a rudimentary spec that defines the conditions we want to test for, and agree that the naming of this field is not the immediate concern.

Production constraints: These are constraints that package managers will take into consideration when installing this package into a dependency tree.
Development constraints. These are constraints that package managers will take into consideration when interacting with this package as an "app" aka installing its dependencies or manually running lifecycle scripts.

Both constraint types have the same identical fields allowed:

Node.js version: this is a single semver compatible range. If the current runtime is not within that range, this constitutes a failure.
- Package managers: This should be a discrete group. Each package manager can be represented by an entry in this set. Name and semver range are required. Other fields can be added (TBD). If the current package manager is not listed by name in this set that is a (warning, failure?) state. If the current package manager is listed by name but the current version is not within the given range, that is a failure state. Other fields can also suggest remediation (this is where the old corepack behavior can be followed if the package manager chooses).
- Failure states: by default a failure should be a warning. Folks can opt into a "strict" mode that will halt operations instead.

This is very much off-the-cuff but hopefully gets someone who feels they are good at writing/developing specs a kernel to start from.

wesleytodd commented 8 months ago

@GeoffreyBooth while I agree those steps would help address the issues with the packageManager field, I think we are still headed down a bad path if we don't include node version requirements in the solution. I think ideally we have an open ended data structure for defining development time dependencies, nearly exactly like the devEngines proposal (so near in fact I would almost just say ship that).

wraithgar commented 8 months ago

So the next step would be to draft such a document and get all sides to agree on it.

Was typing mine up as you posted this.

I assume npm wouldn’t support the “download” option, so it could just error and exit if that’s what’s specified.

Probably not initially. Even if everyone eventually supports it I think it's very valuable to bake into the spec the behavior for when the package manager either chooses not to (or is told by the user not to) install.

Please note that what I added is a suggestion based on my experience as one npm's developers. I know other use cases and requirements are out there and they should be heard. npm does not need to drive this spec but we would like to have input and ultimately we'll need to sign off on it to implement it in npm itself. I would also humbly ask that we don't implement anything without hearing from someone in the yarn and pnpm projects at a minimum.

I can be the primary liaison for npm on this, and I'm sure @lukekarrys will have good input too when he has time.

wraithgar commented 8 months ago

Yes node version has to be part of this. It should explicitly be about Node.js too. Folks may want to also specify Deno, for example. There is also the "browserslist" compatibility flags that seem to have been implemented in various ways over the years. If the spec could at least be designed in a way that leaves room for those later, all the better.

Let's not also forget that we have os libc and cpu that were added as top level fields that solve tangentially similar issues. If we're defining a future-proof all-encompassing spec to define "runtime constraints" we should at least be aware that those fields exist and are in use.

GeoffreyBooth commented 8 months ago

Where should this spec be defined? It looks like there are the foundations of a package.json standardization effort at https://github.com/openjs-foundation/standards/issues/233#issuecomment-1577099979. cc @Ethan-Arrowood

wraithgar commented 8 months ago

strict should not be in the spec. That is a user directive.

wesleytodd commented 8 months ago

Where should this spec be defined?

https://github.com/openjs-foundation/package-metadata-interoperability-collab-space

The collab space has not yet taken on the scope of spec'ing anything, so if this would be the first thing we would need to ensure the other package managers are comfortable with that method of work. That said, longer term that was the goal of the group.

richardlau commented 8 months ago

Where should this spec be defined? It looks like there are the foundations of a package.json standardization effort at openjs-foundation/standards#233 (comment). cc @Ethan-Arrowood

I'd be +1 for seeing if that plays out. Traditionally Node.js has not been dictating what goes into package.json (it used to just be "name" and "main") and mainly left that to the ecosystem, although that has changed somewhat with the introduction of ESM.

wesleytodd commented 8 months ago

If you are interested, we are discussing in the OpenJS Slack #package-meta-interop if that group would be a good place to host a discussion of a shared specification which includes the broader set of needs which would also get the npm team on board.

wraithgar commented 8 months ago

I don't particularly have strong opinions where the discussion happens, as long as it's not in github.com/npm. This is something the community needs to agree on, and npm should not be seen as driving it (similar to your concerns about Node.js not dictating what goes into package.json). We will help as much as we can, and work with folks to come up with a "runtime dependencies" spec that works for the community and is something npm can implement.

I believe I've outlined the base issue from our perspective in my "not a spec" spec mock up, and if you would like me to follow along somewhere else feel free to ping me directly (please don't ping the npm cli group at large, notifications are at a premium these days).

GeoffreyBooth commented 8 months ago

Continuing at https://github.com/openjs-foundation/package-metadata-interoperability-collab-space/issues/15.

nodejs / node