rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

RFC: Peer dependencies should be able to match a full range of prerelease versions

Open alasdairhurst opened this issue 3 years ago • 16 comments

A User should be able to specify a "semver" range, such that all prerelease versions within that range will match.

alasdairhurst avatar Jun 11 '21 13:06 alasdairhurst

Meeting notes discussion: https://github.com/npm/rfcs/blob/latest/meetings/2021-07-28.md#pr-397-rfc-peer-dependencies-should-be-able-to-match-a-full-range-of-prerelease-versions---alasdairhurst

Next action items:

  • [ ] provide more use cases
  • [ ] rather than make this specific to peerDeps, consider adding a --include-prerelease flag to npm, which would tell it to include prerelease versions in all semver matching operations.

isaacs avatar Aug 04 '21 03:08 isaacs

I'll try to clarify the use case I have in mind in (hopefully) simpler terms: (cc @wraithgar)

Example

"library" is being developed using prerelease versions and "library" has a wide range of plugins (let's say there are 100+ of them).

Plugins have varying peer dependencies on "Library". let's take two examples: "library-plugin-1" knows that it works with "[email protected]" and greater. (can't guarantee 5.0.0 and higher for obvious reasons) "library-plugin-2" knows that it works with "[email protected]" and greater (using a feature that was introduced in that version)

[email protected] is in development and not quite ready, so "[email protected]" is released for testing

Personas and issues

The issue is the following on npm 7:

  • "library" developers can't test "[email protected]" with any existing plugins since multiple versions of "[email protected]" will be resolved based on the minimum supported version. They will have to re-publish every plugin they test with. (assuming they're owned by the same developer)
  • "library" users can't test that "[email protected]" works in their application for the same reason. These users don't write plugins or develop "library". They do want to test the latest version to catch issues and report them before the official version gets released
  • "library-plugin" developers can't test "[email protected]" with existing versions of their plugins for regressions, again for the same reason. These developers can change the peer dependencies though, but it's only a temporary solution.

A few things to keep in mind:

  • We can't force every plugin developer to publish a new version of their plugin for every prerelease version published. It's crazy and unmaintainable.
  • This would only apply to specific packages. A global flag to opt-in to all prerelease matches of every module in the tree would be a bit too much.

Suggestion:

I've come up with the following suggestion based on ideas thrown around during the RFC meeting:

My suggestion is either as an opt-in, or even by default, that if pre-release modules ("[email protected]") are directly installed by a package (i.e. an explicit dependency), and that pre-release fits within the semver range provided by a peer dependency (assuming ^4.0.0 includes 4.0.0-0 and 4.1.0-0 but not 5.0.0-0 in this particular calculation), then that particular pre-release peer will be treated as resolved and no other version will be attempted to be installed.

This is specifically an opt-in on behalf of the user who creates the package which depends on both "library" and "library-plugin". Let me know if anyone can think that this would result in unexpected/unwanted behavour.

Alternatively, the same approach applies, but the plugin itself declares in peerDependenciesMeta that prereleases should be accepted in the semver range. This is ok but I don't believe that it's necessary for a few reasons:

  • It's extra work for the plugin developer to remember to do. (remember, it could also impact the person who uses the plugin down the line and not just the plugin developer)
  • It's going to have the same behavior 99% of the time. In a regular case, a user will install "[email protected]" (non-prerelease) and a plugin. The peer will resolve as an existing dependency and happy days. a prerelease version will only ever be installed in 2 cases:

alasdairhurst avatar Aug 04 '21 20:08 alasdairhurst

fwiw, all those categories of people can test it, they just have to use --legacy-peer-deps and temporarily ignore that their dep graph is invalid.

ljharb avatar Aug 05 '21 01:08 ljharb

@ljharb you're absolutely right. This is mentioned as a workaround in the PR, although I'm intentionally ignoring this when discussing the issue since we need to figure out a way for this to work in a modern way, as NPM prefers resolving peers now, and not a legacy one which could be removed at any point in time.

alasdairhurst avatar Aug 05 '21 10:08 alasdairhurst

consider adding a --include-prerelease flag to npm, which would tell it to include prerelease versions in all semver matching operations.

👍🏼

roryabraham avatar Oct 28 '22 11:10 roryabraham

Here's a use case: https://github.com/eslint-community/eslint-utils/issues/183#issuecomment-1987210459

Basically: ESLint plugins / code with a peer dependency saying that it supports everything in ESLint 8 and later won't install cleanly with ESLint 9 pre-releases, which is not the intention of ESLint plugins / code when they say >=8.0.0.

When we in ESLint plugins / code says >=8.0.0 what we would want to happen is:

  • When automatically resolving peer dependency, resolve to latest matching stable
  • When checking if manually installed peer dependency matches, allow pre-releases

My current suggestion in that issue is to pre-emptively do ^8.0.0 || >=9.0.0-0 where 8 is the current major and 9 is the next major, but it has some drawbacks:

  1. It may automatically install a pre-release
  2. It only allows pre-releases of 9.0.0, not of eg. 9.0.1 or 10.0.0, making it an ever moving target

Implementation wise, what I would prefer is:

When checking if a peer dependency is valid, use semver.satisfies('9.0.0-beta.2', '>=8.0.0', { includePrerelease: true }) rather than satisfies('9.0.0-beta.2', '>=8.0.0')

voxpelli avatar Mar 10 '24 13:03 voxpelli

Another use case:

knip has "typescript": ">=5.0.4" in its peerDependencies, which means: "I need at least 5.0.4"

But when used with a pre-release of typescript, such as 5.5.0-beta, npm complains that the >=5.0.4 version range does not permit 5.5.0-beta, which is technically true to the semver spec, but is at odds with the intention of a "typescript": ">=5.0.4" peer dependency

voxpelli avatar Apr 29 '24 10:04 voxpelli

When checking if a peer dependency is valid, use semver.satisfies('9.0.0-beta.2', '>=8.0.0', { includePrerelease: true }) rather than satisfies('9.0.0-beta.2', '>=8.0.0')

I think this check happens here: https://github.com/npm/cli/blob/762888a3b603704c7c53a94a704b8a7f3edea918/workspaces/arborist/lib/dep-valid.js#L53

But it does so for a lot more cases than when checking if a peer dependency is valid

voxpelli avatar Apr 29 '24 12:04 voxpelli

which is technically true to the semver spec

The semver spec does not include range specifiers IIRC, so I think the meaning here is more the semver package does not include prerelease versions right? I wonder if a discussion should happen here?

EDIT: The issue here is that the library allows for specifying includePrerelesese but there is no way for a range specifier string to indicate this. That is the discussion I have not seen happen here or in the semver lib.

wesleytodd avatar Apr 29 '24 14:04 wesleytodd

If you're using a prerelease, I believe you can use overrides and it'll ignore the range - which supports application use cases without leaking things into the module graph.

ljharb avatar Apr 29 '24 15:04 ljharb

without leaking things into the module graph.

There are cases where you want this in the published version. Not as much in open source packages but we have many packages which use "git ops" style workflows where packages live with applications and forcing this for PR builds would be nice. In the current state we end up publishing many more versions because we need to hard code the specific prerelease to get it to work. If we had a feature like this we could publish just the changed package as we iterate on the PR.

wesleytodd avatar Apr 29 '24 16:04 wesleytodd

The semver spec does not include range specifiers IIRC

My memory played big tricks on me 😳 Semver range specifiers should also have a spec!

there is no way for a range specifier string to indicate this. That is the discussion I have not seen happen here or in the semver lib.

I would say that there's no need for a new range specifier. The peer dependency validation logic in npm simply has to change so that a peer dependency range of eg. >=1.2.3 while still requesting the latest stable also accepts any newer pre-release version (and same should be true for eg. ^1.2.3 and ~1.2.3)

So: The acceptance check that happens for peer dependencies should change to have includePrerelesese set as acceptance is different to requesting.


My use case with knip was to use a new version of my personal tsconfig, that uses the TS 5.5.0-beta, and when installing that in a project having npm reject it because knip requiring >=5.0.4, which is not at all the intent from knip, they only say Do not use a version lower than 5.0.4

voxpelli avatar Apr 29 '24 16:04 voxpelli

So: The acceptance check that happens for peer dependencies should change to have includePrerelesese set as acceptance is different to requesting.

This would be a breaking change for sure, and I think my point above is this is a change which is unlikely to land well. It is a tradeoff where this behavior would make a lot of existing uses broken to benefit this smaller use case. Which is why I think the range needs to include it to support both use cases.

I think the use case is totally valid, but it is enough in conflict with the current behavior which is relied upon for other legitimate use cases that I think simply changing it is not the best option.

wesleytodd avatar Apr 29 '24 17:04 wesleytodd

There's an open PR on the semver spec to add ranges (using npm's semantics, ofc).

knip would have to do >= 5.4.0 || ^5.4.0-0 || ^5.5.0-0 etc if it wants to allow prereleases.

ljharb avatar Apr 29 '24 17:04 ljharb

The range grammar spec @ljharb mentioned: https://github.com/semver/semver/pull/584

The goal of that discussion was to document what node-semver does, and then see what overlap or patterns could be codified, maybe changes that could be made to bring us all into compliance with one another, etc.

The outcome was that (a) there really is not much overlap in how different package manager implementations do semver ranges (except for cargo and npm, since cargo copied node-semver's semantics pretty faithfully), and (b) almost any change, no matter how minor, would be an ecosystem-splitting breaking change, so the costs are very high.

So, it seems unlikely that the range specifiers will ever be a more normative spec than just "this is how node-semver does it". Which, ok, still useful maybe. But also, any extension or change to it is likely to have very profound negative consequences. If it's strictly additive, that might be fine, especially if npm prevented publishes using those new range grammar extensions (or converted them, like how pnpm and yarn convert workspace:^ into an actual semver range).

tl;dr - as far as semver range expressions go, most likely we're kinda stuck with what we've got.

Getting back to the OP issue here, I think allowing prereleases in peerDependencies would be fine, and likely not a breaking change to anyone, as long as non-prerelease versions were still prioritized, with one important exception, which probably kills the idea (or at least, imposes a tricky puzzle to be solved).

Let's say that you have "peerDependencies": { "foo": ">=1.0.0 <2.0.0" }

The version 2.0.0-beta is lower precedence than 2.0.0, according to the semver spec. When prereleases are not included, 2.0.0-beta does not satisfy <2.0.0, because the prerelease has not been opted into. However, if we allow prereleases in peerDeps, then 2.0.0-beta does satisfy <2.0.0, because it's lower precedence. This is important in some cases, for example a security advisory that says that the fixed version is 2.0.0 - any prerelease of 2.0.0 might still be vulnerable.

It's reasonable for us humans to look at that peerDeps spec, and see that while they might be ok with 1.2.3-beta, they're clearly not going to be ok with 2.0.0-beta. That means, there are some prereleases that are acceptable, and others that are not.

So, we'd have to somehow unambiguously codify exactly which prereleases should be included, and which should not. I don't think this is impossible, by any means, but it would have to be done in order for this to not be a footgun.

isaacs avatar Apr 29 '24 18:04 isaacs

Maybe a first stab at that codification would be as simple as: "Include any prereleases, but treat all <X.Y.Z as <X.Y.Z-0."

This would solve the case of >=1.0.0 <2.0.0, because it'd be interpreted as >=1.0.0 <2.0.0-0, which excludes 2.0.0-beta explicitly.

I have not exhaustively gone through all the possible cases to verify this doesn't have some other bad effects, though.

isaacs avatar Apr 29 '24 18:04 isaacs