Yarn 1.1.0 optimizeResolutions fails if package pattern has || in the range

[rekotiira]

Commit 4020ccd introduced a feature that still exists in the current version:

As a final step of package resolution, for each dependency we check whether any version satisfies all resolved version ranges.

There is a bug with this feature. If a dependency has a || in its semver range, the code incorrectly resolves to a version that in reality really doesn't satisfy all version ranges.

For example I had the following lodash versions:

lodash@^4.11.1
lodash@~3.5.0
[email protected]
lodash@^3.0.1 || ^2.0.0

And the available versions were '4.17.4', '3.5.0', '2.4.2', '3.10.1'

The code generates a combinedRange: ^4.11.1 ~3.5.0 2.4.2 ^3.0.1 || ^2.0.0

And this combined range incorrectly resolves to a single version: 2.4.2.

The root of the issue is that semver apparently evaluates that expression as: ( ^4.11.1 ~3.5.0 2.4.2 ^3.0.1 ) || ^2.0.0 although the intention of the code is to evaluate it as ^4.11.1 ~3.5.0 2.4.2 ( ^3.0.1 || ^2.0.0 )

It seems that there is no way to group the OR in the latter manner, hence as a fix the patterns with OR could simply be excluded from the optimization. Here's a patch that fixes the issue:

diff --git a/src/package-resolver.js b/src/package-resolver.js
index 8079989..3468ad1 100644
--- a/src/package-resolver.js
+++ b/src/package-resolver.js
@@ -560,10 +560,12 @@ export default class PackageResolver {

     // don't optimize things that already have a lockfile entry:
     // https://github.com/yarnpkg/yarn/issues/79
-    const collapsablePatterns = patterns.filter(pattern => {
-      const remote = this.patterns[pattern]._remote;
-      return !this.lockfile.getLocked(pattern) && (!remote || remote.type !== 'workspace');
-    });
+    const collapsablePatterns = patterns
+      .filter(pattern => {
+        const remote = this.patterns[pattern]._remote;
+        return !this.lockfile.getLocked(pattern) && (!remote || remote.type !== 'workspace');
+      })
+      .filter(pattern => pattern.indexOf('||') === -1);
     if (collapsablePatterns.length < 2) {
       return;
     }

Tested on latest LTS version of node (v6.11.3).

[BYK]

Actually, looks like we can manually craft a range that satisfies our needs: https://github.com/npm/node-semver/blob/master/semver.js#L768

[BYK]

I have a PR cooking for this.