Fix: Resolve peerDependencies from all higher levels, not just root (y…

…arnpkg#4478)

**Summary**

Fixes yarnpkg#4446, fixes yarnpkg#4433, fixes yarnpkg#2688, fixes yarnpkg#2387. Follow up to yarnpkg#3803. The fix in yarnpkg#3893 was
too aggressive, allowing only top-level dependencies to be used in
peer dependency resolution which was incorrect. This patch allows
resolving peer dependencies from the same or higher levels in the
dependency tree.

**Test plan**

Additional unit and integration tests.

__tests__/commands/add.js

@@ -849,7 +849,7 @@ test.concurrent('warns when peer dependency is incorrect during add', (): Promis
   );
 });
 
-test.concurrent('should only refer to root to satisfy peer dependency', (): Promise<void> => {
+test.concurrent('should only refer to higher levels to satisfy peer dependency', (): Promise<void> => {
   return buildRun(
     reporters.BufferReporter,
     fixturesLoc,
@@ -858,20 +858,33 @@ test.concurrent('should only refer to root to satisfy peer dependency', (): Prom
       await add.init();
 
       const output = reporter.getBuffer();
-      const warnings = output.filter(entry => entry.type === 'warning');
-
-      expect(
-        warnings.some(warning => {
-          return warning.data.toString().toLowerCase().indexOf('incorrect peer') > -1;
-        }),
-      ).toEqual(true);
+      const warnings = output.filter(entry => entry.type === 'warning').map(entry => entry.data);
+      expect(warnings).toEqual(expect.arrayContaining([expect.stringContaining('incorrect peer')]));
     },
     ['file:c'],
     {},
     'add-with-multiple-versions-of-peer-dependency',
   );
 });
 
+test.concurrent('should refer to deeper dependencies to satisfy peer dependency', (): Promise<void> => {
+  return buildRun(
+    reporters.BufferReporter,
+    fixturesLoc,
+    async (args, flags, config, reporter, lockfile): Promise<void> => {
+      const add = new Add(args, flags, config, reporter, lockfile);
+      await add.init();
+
+      const output = reporter.getBuffer();
+      const warnings = output.filter(entry => entry.type === 'warning').map(entry => entry.data);
+      expect(warnings).not.toEqual(expect.arrayContaining([expect.stringContaining('peer')]));
+    },
+    [],
+    {},
+    'add-with-deep-peer-dependencies',
+  );
+});
+
 test.concurrent('should retain build artifacts after add when missing integrity file', (): Promise<void> => {
   return buildRun(
     reporters.BufferReporter,

__tests__/fixtures/add/add-with-deep-peer-dependencies/a/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "a",
+  "version": "0.0.0",
+  "dependencies": {
+    "b": "file:../b",
+    "c": "file:../c"
+  }
+}

__tests__/fixtures/add/add-with-deep-peer-dependencies/b/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "b",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "c": "1.0.0"
+  }
+}

__tests__/fixtures/add/add-with-deep-peer-dependencies/c/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "c",
+  "version": "1.0.0"
+}

__tests__/fixtures/add/add-with-deep-peer-dependencies/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "d",
+  "version": "3.0.0",
+  "dependencies": {
+    "a": "file:a"
+  }
+}

__tests__/integration.js

@@ -7,24 +7,32 @@ import * as fs from '../src/util/fs.js';
 import * as misc from '../src/util/misc.js';
 import * as constants from '../src/constants.js';
 
-jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
+jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000;
 
 const path = require('path');
 
-function addTest(pattern) {
-  // concurrently network requests tend to stall
-  test(`yarn add ${pattern}`, async () => {
+function addTest(pattern, {strict} = {strict: false}) {
+  test.concurrent(`yarn add ${pattern}`, async () => {
     const cwd = await makeTemp();
     const cacheFolder = path.join(cwd, 'cache');
 
     const command = path.resolve(__dirname, '../bin/yarn');
-    const args = ['--cache-folder', cacheFolder, '--verbose'];
+    const args = ['--cache-folder', cacheFolder];
 
     const options = {cwd};
 
-    await fs.writeFile(path.join(cwd, 'package.json'), JSON.stringify({name: 'test'}));
+    await fs.writeFile(
+      path.join(cwd, 'package.json'),
+      JSON.stringify({
+        name: 'test',
+        license: 'MIT',
+      }),
+    );
 
-    await execa(command, ['add', pattern].concat(args), options);
+    const result = await execa(command, ['add', pattern].concat(args), options);
+    if (strict) {
+      expect(result.stderr).not.toMatch(/^warning /gm);
+    }
 
     await fs.unlink(cwd);
   });
@@ -41,7 +49,7 @@ function addTest(pattern) {
 //       path.join(folder, constants.METADATA_FILENAME),
 //       '{"remote": {"hash": "cafebabecafebabecafebabecafebabecafebabe"}}',
 //     );
-//     await fs.writeFile(path.join(folder, 'package.json'), '{"name": "@foo/bar", "version": "1.2.3"}');
+//     await fs.writeFile(path.join(foldresolve gitlab:leanlabsio/kanbaner, 'package.json'), '{"name": "@foo/bar", "version": "1.2.3"}');
 //   },
 //   true,
 // ); // offline npm scoped package
@@ -51,6 +59,10 @@ addTest('https://git@github.com/stevemao/left-pad.git'); // git url, with userna
 addTest('https://github.com/yarnpkg/yarn/releases/download/v0.18.1/yarn-v0.18.1.tar.gz'); // tarball
 addTest('https://github.com/bestander/chrome-app-livereload.git'); // no package.json
 addTest('bestander/chrome-app-livereload'); // no package.json, github, tarball
+// Only run `react-scripts` test on Node 6+
+if (parseInt(process.versions.node.split('.')[0], 10) >= 6) {
+  addTest('react-scripts@1.0.13', {strict: true}); // many peer dependencies, there shouldn't be any peerDep warnings
+}
 
 const MIN_PORT_NUM = 56000;
 const MAX_PORT_NUM = 65535;

__tests__/package-resolver.js

@@ -17,8 +17,7 @@ const path = require('path');
 const cachePathRe = /-\d+\.\d+\.\d+-[\dabcdef]{40}$/;
 
 function addTest(pattern, registry = 'npm', init: ?(cacheFolder: string) => Promise<any>, offline = false) {
-  // concurrently network requests tend to stall
-  test(`${offline ? 'offline ' : ''}resolve ${pattern}`, async () => {
+  test.concurrent(`${offline ? 'offline ' : ''}resolve ${pattern}`, async () => {
     const lockfile = new Lockfile();
     const reporter = new reporters.NoopReporter({});
 

src/package-linker.js

@@ -432,34 +432,49 @@ export default class PackageLinker {
 
   resolvePeerModules() {
     for (const pkg of this.resolver.getManifests()) {
-      this._resolvePeerModules(pkg);
-    }
-  }
-
-  _resolvePeerModules(pkg: Manifest) {
-    const peerDeps = pkg.peerDependencies;
-    if (!peerDeps) {
-      return;
-    }
-
-    const ref = pkg._reference;
-    invariant(ref, 'Package reference is missing');
+      const peerDeps = pkg.peerDependencies;
+      if (!peerDeps) {
+        continue;
+      }
+      const ref = pkg._reference;
+      invariant(ref, 'Package reference is missing');
+
+      for (const peerDepName in peerDeps) {
+        const range = peerDeps[peerDepName];
+        const peerPkgs = this.resolver.getAllInfoForPackageName(peerDepName);
+
+        let peerError = 'unmetPeer';
+        let resolvedLevelDistance = Infinity;
+        let resolvedPeerPkgPattern;
+        for (const peerPkg of peerPkgs) {
+          const peerPkgRef = peerPkg._reference;
+          if (!(peerPkgRef && peerPkgRef.patterns)) {
+            continue;
+          }
+          const levelDistance = ref.level - peerPkgRef.level;
+          if (levelDistance >= 0 && levelDistance < resolvedLevelDistance) {
+            if (this._satisfiesPeerDependency(range, peerPkgRef.version)) {
+              resolvedLevelDistance = levelDistance;
+              resolvedPeerPkgPattern = peerPkgRef.patterns;
+              this.reporter.verbose(
+                this.reporter.lang(
+                  'selectedPeer',
+                  `${pkg.name}@${pkg.version}`,
+                  `${peerDepName}@${range}`,
+                  peerPkgRef.level,
+                ),
+              );
+            } else {
+              peerError = 'incorrectPeer';
+            }
+          }
+        }
 
-    for (const name in peerDeps) {
-      const range = peerDeps[name];
-      const pkgs = this.resolver.getAllInfoForPackageName(name);
-      const found = pkgs.find(pkg => {
-        const {root, version} = pkg._reference || {};
-        return root && this._satisfiesPeerDependency(range, version);
-      });
-      const foundPattern = found && found._reference && found._reference.patterns;
-
-      if (foundPattern) {
-        ref.addDependencies(foundPattern);
-      } else {
-        const depError = pkgs.length > 0 ? 'incorrectPeer' : 'unmetPeer';
-        const [pkgHuman, depHuman] = [`${pkg.name}@${pkg.version}`, `${name}@${range}`];
-        this.reporter.warn(this.reporter.lang(depError, pkgHuman, depHuman));
+        if (resolvedPeerPkgPattern) {
+          ref.addDependencies(resolvedPeerPkgPattern);
+        } else {
+          this.reporter.warn(this.reporter.lang(peerError, `${pkg.name}@${pkg.version}`, `${peerDepName}@${range}`));
+        }
       }
     }
   }

src/package-reference.js

@@ -27,7 +27,7 @@ export default class PackageReference {
     this.permissions = {};
     this.patterns = [];
     this.optional = null;
-    this.root = false;
+    this.level = Infinity;
     this.ignore = false;
     this.incompatible = false;
     this.fresh = false;
@@ -39,7 +39,7 @@ export default class PackageReference {
   lockfile: Lockfile;
   config: Config;
 
-  root: boolean;
+  level: number;
   name: string;
   version: string;
   uid: string;
@@ -66,9 +66,13 @@ export default class PackageReference {
   addRequest(request: PackageRequest) {
     this.requests.push(request);
 
-    if (!request.parentRequest) {
-      this.root = true;
+    let requestLevel = -1;
+    let currentRequest = request;
+    while (currentRequest && requestLevel < this.level) {
+      requestLevel += 1;
+      currentRequest = currentRequest.parentRequest;
     }
+    this.level = requestLevel;
   }
 
   prune() {

src/reporters/lang/en.js

@@ -223,6 +223,7 @@ const messages = {
 
   unmetPeer: '$0 has unmet peer dependency $1.',
   incorrectPeer: '$0 has incorrect peer dependency $1.',
+  selectedPeer: 'Selecting $1 at level $2 as the peer dependency of $0.',
   missingBundledDependency: '$0 is missing a bundled dependency $1. This should be reported to the package maintainer.',
 
   savedNewDependency: 'Saved 1 new dependency.',