[QUESTION] semver.intersects('>=16.0.0 <17.0.0', '^17.0.0-0', { includePrereleases: true }) should be false

[ljharb]

What / Why

semver.intersects('>=16.0.0 <17.0.0', '^17.0.0-0') should be true, and is.

semver.intersects('>=16.0.0 <17.0.0', '^17.0.0-0', { includePrereleases: true }) should be false, and is not.

I made test cases:

diff --git a/test/classes/range.js b/test/classes/range.js
index e3867a9..bf77331 100644
--- a/test/classes/range.js
+++ b/test/classes/range.js
@@ -83,7 +83,7 @@ test('tostrings', (t) => {
 })
 
 test('ranges intersect', (t) => {
-  rangeIntersection.forEach(([r0, r1, expect]) => {
+  rangeIntersection.forEach(([r0, r1, expect, includePrerelease]) => {
     t.test(`${r0} <~> ${r1}`, t => {
       const range0 = new Range(r0)
       const range1 = new Range(r1)
@@ -92,6 +92,14 @@ test('ranges intersect', (t) => {
         `${r0} <~> ${r1} objects`)
       t.equal(range1.intersects(range0), expect,
         `${r1} <~> ${r0} objects`)
+
+      if (includePrerelease) {
+        t.equal(range0.intersects(range1, { includePrerelease: true }), expect,
+          `${r0} <~> ${r1} objects`)
+        t.equal(range1.intersects(range0, { includePrerelease: true }), expect,
+          `${r1} <~> ${r0} objects`)
+      }
+
       t.end()
     })
   })
diff --git a/test/fixtures/range-intersection.js b/test/fixtures/range-intersection.js
index e46c82a..956de82 100644
--- a/test/fixtures/range-intersection.js
+++ b/test/fixtures/range-intersection.js
@@ -53,5 +53,7 @@ module.exports = [
   ['1.3.0 || <1.0.0 >2.0.0', 'x', true],
   ['1.x', '1.3.0 || <1.0.0 >2.0.0', true],
   ['*', '*', true],
-  ['x', '', true]
+  ['x', '', true],
+  ['>=16.0.0 <17.0.0', '^17.0.0-0', false],
+  ['>=16.0.0 <17.0.0', '^17.0.0-0', true, true],
 ]
diff --git a/test/ranges/intersects.js b/test/ranges/intersects.js
index e93492b..1545420 100644
--- a/test/ranges/intersects.js
+++ b/test/ranges/intersects.js
@@ -28,7 +28,7 @@ test('intersect comparators', t => {
 })
 
 test('ranges intersect', (t) => {
-  rangeIntersection.forEach(([r0, r1, expect]) => {
+  rangeIntersection.forEach(([r0, r1, expect, includePrerelease]) => {
     t.test(`${r0} <~> ${r1}`, t => {
       const range0 = new Range(r0)
       const range1 = new Range(r1)
@@ -43,6 +43,20 @@ test('ranges intersect', (t) => {
         `${r0} <~> ${r1} objects loose`)
       t.equal(intersects(range1, range0, true), expect,
         `${r1} <~> ${r0} objects loose`)
+
+      if (includePrerelease) {
+        t.equal(intersects(r1, r0, { includePrerelease: true }), expect, `${r0} <~> ${r1}`)
+        t.equal(intersects(r0, r1, { includePrerelease: true }), expect, `${r1} <~> ${r0}`)
+        t.equal(intersects(r1, r0, { loose: true, includePrerelease: true }), expect, `${r0} <~> ${r1} loose`)
+        t.equal(intersects(r0, r1, { loose: true, includePrerelease: true }), expect, `${r1} <~> ${r0} loose`)
+        t.equal(intersects(range0, range1, { includePrerelease: true }), expect, `${r0} <~> ${r1} objects`)
+        t.equal(intersects(range1, range0, { includePrerelease: true }), expect, `${r1} <~> ${r0} objects`)
+        t.equal(intersects(range0, range1, { loose: true, includePrerelease: true }), expect,
+          `${r0} <~> ${r1} objects loose`)
+        t.equal(intersects(range1, range0, { loose: true, includePrerelease: true }), expect,
+          `${r1} <~> ${r0} objects loose`)
+      }
+
       t.end()
     })
   })

but i can't figure out how to fix it.

[isaacs]

Hm, I disagree with the stated intent here.

semver.intersects('>1.0.0 <2.0.0', '^2.0.0-0', {includePrerelease: false}) should be false. There is no version that satisfies both ranges, because 2.0.0-0 is not included by <2.0.0 if includePrerelease is not enabled.

semver.intersects('>1.0.0 <2.0.0', '^2.0.0-0', {includePrerelease: true}) should be true. 2.0.0-0 is included by both ranges in includePrerelease mode.

[ljharb]

Ah ok, I can swap them - that makes sense to me, I just misunderstood our slack discussion. I just need it to be true with one of the options :-)

Updated.

[lukekarrys]

Action Item:

• make semver.intersects('>1.0.0 <2.0.0', '^2.0.0-0', {includePrerelease: false}) === false

[mbtools]

Same comment as here:

Comparison functions -which are ultimately called by intersects - do not use includePrerelease but always compare the pre-release parts as well.

[ljharb]

@mbtools if it accepts the option, it should obey it - iow, the bug is either that it doesn't obey the option, or that it accepts it, but either way there's a bug.

[mbtools]

It comes down to this:

cmp = new SemVer('1.2.3', { includePrerelease: false }).compare('1.2.3-10')
// 1, not equal
cmp = new SemVer('1.2.3', { includePrerelease: true }).compare('1.2.3-10')
// 1, not equal

I don't think we can change the constructor. So are we saying the default behaviour of compare and therefore all features that depend on it have a bug?

[ljharb]

Yes, I'd say that when the includePrerelease option was introduced, every API should have either been made to obey it or throw when it was going to disobey it.

Separately, I'm not sure why the constructors need to constrain the functions - i'm not using the instances at all in my OP (the implementation detail that it's using them internally isn't something I should need to think about)