-
Notifications
You must be signed in to change notification settings - Fork 239
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
base: main
Are you sure you want to change the base?
Conversation
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:
|
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@4.6.0 is in development and not quite ready, so "library@4.6.0-1" is released for testing Personas and issuesThe issue is the following on npm 7:
A few things to keep in mind:
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:
|
fwiw, all those categories of people can test it, they just have to use |
@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. |
👍🏼 |
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 When we in ESLint plugins / code says
My current suggestion in that issue is to pre-emptively do
Implementation wise, what I would prefer is: When checking if a peer dependency is valid, use |
Another use case:
But when used with a pre-release of |
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 |
The semver spec does not include range specifiers IIRC, so I think the meaning here is more the EDIT: The issue here is that the library allows for specifying |
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. |
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. |
My memory played big tricks on me 😳 Semver range specifiers should also have a spec!
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. So: The acceptance check that happens for peer dependencies should change to have My use case with |
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. |
There's an open PR on the semver spec to add ranges (using npm's semantics, ofc). knip would have to do |
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 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 The version It's reasonable for us humans to look at that peerDeps spec, and see that while they might be ok with 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. |
Maybe a first stab at that codification would be as simple as: "Include any prereleases, but treat all This would solve the case of I have not exhaustively gone through all the possible cases to verify this doesn't have some other bad effects, though. |
I have the same issue as mentioned in: npm/cli#2087 Is there any solution besides overrides at the moment? There should be a flag to include all the prereleases with a range. |
This mostly works, but fails if someone writes something strange like Depending on how conservative you want to be you can resolve this by choosing one or both of the following rules:
(I mentioned x-ranges above, note that using explicit pre-release versions with x-ranges is currently broken in node-semver: npm/node-semver#510 ) |
How about this approach? Add an For example: "peerDependencies": {
"foo": "2.x",
},
"peerDependenciesMeta": {
"foo": {
"includePrerelease": true
}
} Looks to me like this would solve a large percentage of use cases without having to meddle with the range spec. |
to work around the lack of pre-release support in modern resolution. see npm/rfcs#397
Meddling with the npm resolution behaviour does have two major advantages over an opt-in "includePrerelease":
And I think that there is an important property of the proposed meddling which makes the compatibility risk relatively low. We can say that if it is possible to resolve a set of dependencies without allowing prereleases in peer-dependency ranges, then this should be done preferentially. Meaning that for any existing package with a valid dependency set, the resolution of versions for those dependencies should not change. Only for packages which fail to resolve without allowing prerelease peer deps should behaviour change to become more lenient. |
A User should be able to specify a "semver" range, such that all prerelease versions within that range will match.