Skip to content

fix: handle OR-range unions in subset() (#703)#854

Open
abhu85 wants to merge 1 commit intonpm:mainfrom
abhu85:fix/703-subset-or-union
Open

fix: handle OR-range unions in subset() (#703)#854
abhu85 wants to merge 1 commit intonpm:mainfrom
abhu85:fix/703-subset-or-union

Conversation

@abhu85
Copy link
Copy Markdown
Contributor

@abhu85 abhu85 commented Apr 29, 2026

Summary

Fixes #703

subset('>=17.2.0', '^17.2.0 || >17') incorrectly returns false.

The root cause is that subset() checks each OR-branch of the dom range independently. No single branch of ^17.2.0 || >17 covers >=17.2.0 on its own, but their union does:

^17.2.0  →  [17.2.0, 18.0.0)
>17      →  [18.0.0, ∞)
union    →  [17.2.0, ∞)  ⊇  [17.2.0, ∞)  ✓

Approach

When no single dom OR-branch covers a sub comparator set, fall back to an interval-sweep algorithm:

  1. Extract [lower, upper) bounds from the sub and every dom OR-branch
  2. Sort dom intervals by lower bound (ties broken by operator strictness)
  3. Sweep left-to-right: start at sub's lower bound and greedily extend coverage through overlapping/adjacent dom intervals
  4. If the sweep reaches or exceeds sub's upper bound, the union covers the sub

Key details:

  • Prerelease adjacency: <18.0.0-0 and >=18.0.0 are treated as adjacent in non-prerelease mode (no release version exists between them)
  • Null-set branches (e.g., >5.0.0 <3.0.0) are detected and skipped
  • * / ANY bounds are handled as [-∞, +∞)
  • The fallback only activates when dom has multiple OR-branches

Test plan

  • Original bug: subset('>=17.2.0', '^17.2.0 || >17') now returns true
  • 42 new test cases covering: multi-branch unions, gaps, overlapping intervals, prerelease adjacency, null-set branches, * ranges, inclusive/exclusive bounds, includePrerelease option
  • All existing tests continue to pass (no regressions)
  • 100% code coverage (statements, branches, functions, lines)
  • Lint passes

@abhu85 abhu85 requested a review from a team as a code owner April 29, 2026 19:33
Comment thread ranges/subset.js
// No single dom range covers this sub range, but the union of
// multiple dom ranges might. Only attempt this when there are
// multiple OR branches in the dom.
if (dom.set.length > 1 && unionSubset(simpleSub, dom, options)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bypasses the prerelease-tuple admission rule that simpleSubset enforces above. So subset('>=1.0.0-pre', '<2.0.0 || >=1.0.0') will return true in non-includePrerelease mode

Comment thread test/ranges/subset.js
['>=1.0.0 <3.0.0', '1.0.0 || 2.0.0', false], // dom is all eq-sets
['>=0.0.0', '<2.0.0 || >=1.0.0', true], // dom branch with -infinity lower
['>=1.0.0 <10.0.0', '>=1.0.0 <3.0.0 || >=5.0.0 <7.0.0 || >=9.0.0', false], // gap
['>=0.0.0-0', '* || >=1.0.0', true, { includePrerelease: true }],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding negative tests here for the prerelease lower/upper bounds in non-includePrerelease mode would be great to make sure a regression doesn't happen.

Comment thread ranges/subset.js
for (const simpleDom of dom.set) {
const b = extractBounds(simpleDom, options)
if (b) {
domIntervals.push(b)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eq-only branches are dropped here, so a singleton like 2.0.0 can't bridge <2.0.0 and >2.0.0 in the union

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.

[BUG] subset('>=17.2.0', '^17.2.0 || >17') should be true

2 participants