Fixes how peer dependencies are resolved multiple levels deep (#6443)

yarnpkg · Sep 28, 2018 · 83b5fbc · 83b5fbc
1 parent a7ee6da
commit 83b5fbc
Show file tree

Hide file tree

Showing 8 changed files with 124 additions and 7 deletions.
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl0-1.0.0/index.js b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl0-1.0.0/index.js
@@ -0,0 +1,10 @@
+/* @flow */
+
+module.exports = require(`./package.json`);
+
+for (const key of [`dependencies`, `devDependencies`, `peerDependencies`]) {
+  for (const dep of Object.keys(module.exports[key] || {})) {
+    // $FlowFixMe The whole point of this file is to be dynamic
+    module.exports[key][dep] = require(dep);
+  }
+}
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl0-1.0.0/package.json b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl0-1.0.0/package.json
@@ -0,0 +1,8 @@
+{
+    "name": "peer-deps-lvl0",
+    "version": "1.0.0",
+    "dependencies": {
+        "no-deps": "1.0.0",
+        "peer-deps-lvl1": "1.0.0"
+    }
+}
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl1-1.0.0/index.js b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl1-1.0.0/index.js
@@ -0,0 +1,10 @@
+/* @flow */
+
+module.exports = require(`./package.json`);
+
+for (const key of [`dependencies`, `devDependencies`, `peerDependencies`]) {
+  for (const dep of Object.keys(module.exports[key] || {})) {
+    // $FlowFixMe The whole point of this file is to be dynamic
+    module.exports[key][dep] = require(dep);
+  }
+}
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl1-1.0.0/package.json b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl1-1.0.0/package.json
@@ -0,0 +1,10 @@
+{
+    "name": "peer-deps-lvl1",
+    "version": "1.0.0",
+    "dependencies": {
+        "peer-deps-lvl2": "1.0.0"
+    },
+    "peerDependencies": {
+        "no-deps": "*"
+    }
+}
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl2-1.0.0/index.js b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl2-1.0.0/index.js
@@ -0,0 +1,10 @@
+/* @flow */
+
+module.exports = require(`./package.json`);
+
+for (const key of [`dependencies`, `devDependencies`, `peerDependencies`]) {
+  for (const dep of Object.keys(module.exports[key] || {})) {
+    // $FlowFixMe The whole point of this file is to be dynamic
+    module.exports[key][dep] = require(dep);
+  }
+}
diff --git a/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl2-1.0.0/package.json b/packages/pkg-tests/pkg-tests-fixtures/packages/peer-deps-lvl2-1.0.0/package.json
@@ -0,0 +1,7 @@
+{
+    "name": "peer-deps-lvl2",
+    "version": "1.0.0",
+    "peerDependencies": {
+        "no-deps": "*"
+    }
+}
diff --git a/packages/pkg-tests/pkg-tests-specs/sources/basic.js b/packages/pkg-tests/pkg-tests-specs/sources/basic.js
@@ -284,6 +284,51 @@ module.exports = (makeTemporaryEnv: PackageDriver) => {
       ),
     );
 
+    test(
+      `it should install in such a way that peer dependencies can be resolved (two levels deep)`,
+      makeTemporaryEnv(
+        {
+          dependencies: {[`peer-deps-lvl0`]: `1.0.0`},
+        },
+        async ({path, run, source}) => {
+          await run(`install`);
+
+          await expect(source(`require('peer-deps-lvl0')`)).resolves.toMatchObject({
+            name: `peer-deps-lvl0`,
+            version: `1.0.0`,
+            dependencies: {
+              [`peer-deps-lvl1`]: {
+                name: `peer-deps-lvl1`,
+                version: `1.0.0`,
+                dependencies: {
+                  [`peer-deps-lvl2`]: {
+                    name: `peer-deps-lvl2`,
+                    version: `1.0.0`,
+                    peerDependencies: {
+                      [`no-deps`]: {
+                        name: `no-deps`,
+                        version: `1.0.0`,
+                      },
+                    },
+                  },
+                },
+                peerDependencies: {
+                  [`no-deps`]: {
+                    name: `no-deps`,
+                    version: `1.0.0`,
+                  },
+                },
+              },
+              [`no-deps`]: {
+                name: `no-deps`,
+                version: `1.0.0`,
+              },
+            },
+          });
+        },
+      ),
+    );
+
     test(
       `it should cache the loaded modules`,
       makeTemporaryEnv(

diff --git a/src/util/generate-pnp-map.js b/src/util/generate-pnp-map.js
@@ -202,8 +202,12 @@ async function getPackageInformationStores(
     return {pkg, ref, loc};
   };
 
-  const visit = async (seedPatterns: Array<string>, parentData: Array<string> = []) => {
-    const resolutions = new Map();
+  const visit = async (
+    precomputedResolutions: Map<string, string>,
+    seedPatterns: Array<string>,
+    parentData: Array<string> = [],
+  ) => {
+    const resolutions = new Map(precomputedResolutions);
     const locations = new Map();
 
     // This first pass will compute the package reference of each of the given patterns
@@ -337,11 +341,14 @@ async function getPackageInformationStores(
         return !pkg || !peerDependencies.has(pkg.name);
       });
 
-      // We do this in two steps to prevent cyclic dependencies from looping indefinitely
+      // We inject the partial information in the store right now so that we won't cycle indefinitely
       packageInformationStore.set(packageReference, packageInformation);
-      packageInformation.packageDependencies = await visit(directDependencies, [packageName, packageReference]);
 
-      // We now have to inject the peer dependencies
+      // We must inject the peer dependencies before iterating; one of our dependencies might have a peer dependency
+      // on one of our peer dependencies, so it must be available from the start (we don't have to do that for direct
+      // dependencies, because the "visit" function that will iterate over them will automatically add the to the
+      // candidate resolutions as part of the first step, cf above)
+
       for (const dependencyName of peerDependencies) {
         const dependencyReference = resolutions.get(dependencyName);
 
@@ -350,6 +357,16 @@ async function getPackageInformationStores(
         }
       }
 
+      const childResolutions = await visit(packageInformation.packageDependencies, directDependencies, [
+        packageName,
+        packageReference,
+      ]);
+
+      // We can now inject into our package the resolutions we got from the visit function
+      for (const [name, reference] of childResolutions.entries()) {
+        packageInformation.packageDependencies.set(name, reference);
+      }
+
       // Finally, unless a package depends on a previous version of itself (that would be weird but correct...), we
       // inject them an implicit dependency to themselves (so that they can require themselves)
       if (!packageInformation.packageDependencies.has(packageName)) {
@@ -389,7 +406,7 @@ async function getPackageInformationStores(
 
       packageInformationStore.set(pkg.version, {
         packageLocation: normalizeDirectoryPath(loc),
-        packageDependencies: await visit(ref.dependencies, [name, pkg.version]),
+        packageDependencies: await visit(new Map(), ref.dependencies, [name, pkg.version]),
       });
     }
   }
@@ -403,7 +420,7 @@ async function getPackageInformationStores(
         null,
         {
           packageLocation: normalizeDirectoryPath(config.lockfileFolder),
-          packageDependencies: await visit(seedPatterns),
+          packageDependencies: await visit(new Map(), seedPatterns),
         },
       ],
     ]),