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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alasdairhurst
Copy link

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

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Jul 28, 2021
@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Jul 28, 2021
@isaacs
Copy link
Contributor

isaacs commented Aug 4, 2021

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.

@alasdairhurst
Copy link
Author

alasdairhurst commented Aug 4, 2021

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 "library@4.0.0" and greater. (can't guarantee 5.0.0 and higher for obvious reasons)
"library-plugin-2" knows that it works with "library@4.5.0" and greater (using a feature that was introduced in that version)

library@4.6.0 is in development and not quite ready, so "library@4.6.0-1" is released for testing

Personas and issues

The issue is the following on npm 7:

  • "library" developers can't test "library@4.6.0-1" with any existing plugins since multiple versions of "library@4.x" 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 "library@4.6.0-1" 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 "library@4.6.0-1" 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 ("library@4.0.0-0") 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 "library@4.x" (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:
    • The user installs "library@4.0.0-0" directly
    • The plugin has a peer of a pre-release, and it's the latest "library@4.0.0-0"

@ljharb
Copy link
Contributor

ljharb commented Aug 5, 2021

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.

@alasdairhurst
Copy link
Author

alasdairhurst commented Aug 5, 2021

@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.

@wraithgar wraithgar added the Agenda will be discussed at the Open RFC call label Aug 5, 2021
@darcyclarke darcyclarke added Agenda will be discussed at the Open RFC call and removed Agenda will be discussed at the Open RFC call labels Aug 11, 2021
@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Aug 25, 2021
@roryabraham
Copy link

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

👍🏼

@voxpelli
Copy link

voxpelli commented Mar 10, 2024

Here's a use case: eslint-community/eslint-utils#183 (comment)

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
Copy link

voxpelli commented Apr 29, 2024

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
Copy link

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

@wesleytodd
Copy link

wesleytodd commented Apr 29, 2024

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.

@ljharb
Copy link
Contributor

ljharb commented Apr 29, 2024

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.

@wesleytodd
Copy link

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.

@voxpelli
Copy link

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

@wesleytodd
Copy link

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.

@ljharb
Copy link
Contributor

ljharb commented Apr 29, 2024

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.

@isaacs
Copy link
Contributor

isaacs commented Apr 29, 2024

The range grammar spec @ljharb mentioned: semver/semver#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
Copy link
Contributor

isaacs commented Apr 29, 2024

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.

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 this pull request may close these issues.

None yet

8 participants