Repository navigation
fix: recognize upper-bound-only subsets - #950
Open
Narek-Khachikyan wants to merge 3 commits into
Open
Narek-Khachikyan wants to merge 3 commits into
Narek-Khachikyan wants to merge 3 commits into
Conversation
Upper-bound-only ranges still have a minimum valid version. Normalize that bound before comparing ranges, and compare exclusive stable upper bounds as prerelease-zero bounds when prereleases are excluded. Fixes npm#949
1AbdullahButt
left a comment
There was a problem hiding this comment.
I read this against ranges/subset.js on main. Three notes:
gtis now assumed to be set inside thedomloop. Theif (gt)guard around the>/>=handling is gone, sohigherGT(gt, c)andgt.operatorrun unconditionally. As far as I can tell that's safe:eqSet.size === 1returns before the loop,lt-only now synthesisesgt, and a sub with no comparators is the*special case handled earlier. But that's an implicit invariant spread over ~60 lines. A one-line comment at the loop (or anassert-style earlyreturnif!gt) would stop a future change to the EQ branch from turning intoTypeError: Cannot read properties of undefined (reading 'operator').- The
-0normalisation isn't in the algorithm comment. You added "If LT is set without GT or EQ, use the lowest admitted version for GT" to the header, but the rewrite of an exclusive stable<Xinto<X-0(when!includePrerelease) also changes semantics and is what makes['<1.0.0', '0.x', true]pass. A matching bullet would keep the header comment accurate, since it is effectively the spec for this function. sawNonNullmoved into the outer loop. I agree with this: onmain, a non-null earlier branch could make a later, entirely-null branch returnfalse. It's a separate behaviour change from #949, though. The['<0.0.0 || <0.0.0', '1.0.0', true]style tests cover it, but it'd be worth a sentence in the PR description so it isn't missed in the changelog.
The test table is thorough, especially the includePrerelease mirror cases and the loose <01.0.0 ones.
Static review; I didn't run the suite here.
Reviewed with help from Claude Code.
Author
|
@1AbdullahButt Addressed in fd4f520:
This commit only adds comments. Locally, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upper-bound-only ranges such as
<2.0.0incorrectly fail subset checks against*, and<1.0.0fails against0.x.Use the lowest admitted version for the omitted lower bound and normalize exclusive stable upper bounds when prereleases are excluded. Track empty subset branches independently so they do not change a union's result.
Verified with
npm test: all 54 files pass with 100% coverage. Separate lint, template, and commit-message checks also pass.References
Fixes #949.
Related to #919.