corepack icon indicating copy to clipboard operation
corepack copied to clipboard

`packageManager` field is too limited

Open GeoffreyBooth opened this issue 1 year ago • 13 comments

The packageManager field as currently designed is too limited for what I want to do:

  • As a project author, I want to enforce a version range, like npm >= 10.0.0.

  • As a project author, I want to control what happens when the developer uses a package manager that fails validation, either because it’s the wrong package manager or a version that’s outside of the defined range:

    • Should it warn, but then proceed
    • Should it warn, and then prompt to download the newest supported version, and then try again in that version
    • Should it error, and exit (like engines.strict)
  • As a project author, I want to control over package managers based on operations. For example, I want to ensure that install commands use the package manager I specify, but run commands can use any package manager. (So install via pnpm and run scripts via Bun, for example.)

Seeing as this much complexity will require multiple configuration options, I suggest we retire the packageManager field and create a new corepack field that can contain a configuration object with all the necessary fields to cover the various permutations described above.

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

GeoffreyBooth avatar Feb 27 '24 05:02 GeoffreyBooth

This issue is about several independent feature requests ("I want to use different package managers on different commands", "I want to specify what happens when the version mismatches"). The packageManager field itself has no impact on those, and it makes little sense to start the discussion from that.

Can you close this thread and open dedicated ones for each of those two use cases?

arcanis avatar Feb 27 '24 06:02 arcanis

The request is that I want more configuration options for Corepack. Specifically, these are the things I want to be able to configure. I can split it into multiple issues, but I think these all go together (many of these ideas are current features of the engines field) and the question is really “what use cases do we want to support” which would lead to an update of the design doc (#401).

GeoffreyBooth avatar Feb 27 '24 06:02 GeoffreyBooth

packageManager is not a replacement for the engines field, its goal is more narrow (and that’s probably a good thing)

aduh95 avatar Feb 27 '24 06:02 aduh95

is not a replacement for the engines field

It should be. The engines field can't do some of these things, and for the parts that overlap there should be a clear precedence rather than the two being mixed.

GeoffreyBooth avatar Feb 27 '24 07:02 GeoffreyBooth

its goal is more narrow

Where is the goal defined?

GeoffreyBooth avatar Feb 27 '24 07:02 GeoffreyBooth

its goal is more narrow

Where is the goal defined?

https://nodejs.org/api/packages.html#packagemanager

aduh95 avatar Feb 27 '24 07:02 aduh95

All that says is "The "packageManager" field defines which package manager is expected to be used when working on the current project." Are my use cases in scope or out of scope? You're saying its scope is limited but I don't see a limiting principle here.

GeoffreyBooth avatar Feb 27 '24 14:02 GeoffreyBooth

Cross linking this other comment from GeoffreyBooth since it has a suggestion how to implement:

  • https://github.com/nodejs/node/issues/51888#issuecomment-1965605286

I do wonder if we are going to hit a wall in the future trying to shove everything into one string.

We already have several things defined in one string today: name, version, hash

styfle avatar Feb 27 '24 15:02 styfle

I mean this field is already not enough to define what we need to properly develop a project because it doesn't include what node version you should develop with. IMO trying to fix packageManager is the wrong course and we should deprecate it and move to a more holistic approach to binary management which is more generic to account for the many requirements real projects have.

I know my comment is derailing this issue, but I think it is a big waste of everyone's time to push forward on a bad plan.

wesleytodd avatar Feb 27 '24 16:02 wesleytodd

I do wonder if we are going to hit a wall in the future trying to shove everything into one string.

💯

Or more to the point, it’s too limiting. Maybe we might never want to implement #406, where we can define different package manager possibilities based on operation (or maybe say that we want Corepack validation to apply only to operations that affect lockfiles) but if we’re limiting ourselves to just a single string, it’s awfully hard to potentially someday add such a feature. (Sure, the string could be stringified JSON, but that’s just ridiculous 😄) Or if we want to implement #405, mimicking npm’s engines.strict; I can easily see that being a feature that users want.

Better to shift to something new now, before we’re enabled by default and stable. I like top-level "corepack", or another option is engines.packageManager; in either case, the value would be an object that can continue expanding as we add more options.

GeoffreyBooth avatar Feb 27 '24 16:02 GeoffreyBooth

It looks like the npm team is interested in collaborating on whatever this new configuration would be: https://github.com/nodejs/node/issues/51888#issuecomment-1967141092.

So maybe we don’t name it corepack; we can decide the name later. But I think let’s open this up and define something in collaboration with them, so that hopefully we can spec out something that both Corepack and npm can support, so that from the user’s perspective npm is compatible. Does that work for you @styfle @arcanis @aduh95 ?

GeoffreyBooth avatar Feb 27 '24 17:02 GeoffreyBooth

Just wanted to note that corepack now sets the field by default which will definitely be an issue as soon as more people start using it. If packageManager needs to change in (a near) future this needs to be reverted (see corepack issue https://github.com/nodejs/corepack/issues/485 )

Lectem avatar Jul 14 '24 09:07 Lectem

I think a lot of the the friction here would be resolved by supporting semver ranges? Already open issue:

  • https://github.com/nodejs/corepack/issues/95

legobeat avatar Jul 16 '24 22:07 legobeat