From 4402b3b91cd85c183156a0521ce7d16f8fe271df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 7 Aug 2017 11:47:39 +0100 Subject: [PATCH] Fix: Prevents hoisting through peer dependencies (#4086) **Summary** Fixes #3933. Currently, the code hoists depedencies without ensuring hoisting doesn't break the final structure's `peerDependencies` requirements. This patch fixes that by adding extra checks before hoisting. It comes with a little performance impact but shouldn't be too much. **Test plan** A new integration test that simulates the reported behavior and ensures `yarn` now behaves correctly. --- __tests__/commands/install/integration.js | 6 ++ .../a/index.js | 1 + .../a/package.json | 1 + .../b-1/package.json | 1 + .../b-2/package.json | 1 + .../c/index.js | 1 + .../c/package.json | 1 + .../d/package.json | 1 + .../e/package.json | 1 + .../index.js | 1 + .../package.json | 1 + .../yarn.lock | 28 ++++++++++ src/package-hoister.js | 56 ++++++++++++++++++- 13 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/index.js create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-1/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-2/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/index.js create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/d/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/e/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/index.js create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/package.json create mode 100644 __tests__/fixtures/install/install-should-not-hoist-through-peer-deps/yarn.lock diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index 78ae21df34..a6535cccf5 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -49,6 +49,12 @@ async function mockConstants(base: Config, mocks: Object, cb: (config: Config) = beforeEach(request.__resetAuthedRequests); afterEach(request.__resetAuthedRequests); +test.concurrent('install should not hoist packages above their peer dependencies', async () => { + await runInstall({}, 'install-should-not-hoist-through-peer-deps', async (config): Promise => { + expect(await fs.exists(`${config.cwd}/node_modules/a/node_modules/c`)).toEqual(true); + }); +}); + test.concurrent('install optional subdependencies by default', async () => { await runInstall({}, 'install-optional-dependencies', async (config): Promise => { expect(await fs.exists(`${config.cwd}/node_modules/dep-b`)).toEqual(true); diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/index.js b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/index.js new file mode 100644 index 0000000000..34b467df08 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/index.js @@ -0,0 +1 @@ +require("c") diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/package.json new file mode 100644 index 0000000000..f529327d85 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/a/package.json @@ -0,0 +1 @@ +{"name":"a", "version":"1.0.0", "dependencies":{"b":"file:../b-2", "c":"file:../c"}} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-1/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-1/package.json new file mode 100644 index 0000000000..cd093c8dd5 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-1/package.json @@ -0,0 +1 @@ +{"name":"b", "version":"1.0.0"} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-2/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-2/package.json new file mode 100644 index 0000000000..f40ab68f85 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/b-2/package.json @@ -0,0 +1 @@ +{"name":"b", "version":"2.0.0"} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/index.js b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/index.js new file mode 100644 index 0000000000..ce40f0d3b5 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/index.js @@ -0,0 +1 @@ +console.log(require("b/package.json").version) diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/package.json new file mode 100644 index 0000000000..4411f6101b --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/c/package.json @@ -0,0 +1 @@ +{"name":"c", "version":"1.0.0", "peerDependencies":{"b":"2.0.0"}} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/d/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/d/package.json new file mode 100644 index 0000000000..edc77ecb85 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/d/package.json @@ -0,0 +1 @@ +{"name":"d", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/e/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/e/package.json new file mode 100644 index 0000000000..0362069229 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/e/package.json @@ -0,0 +1 @@ +{"name":"e", "version":"1.0.0", "dependencies":{"b":"file:../b-1"}} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/index.js b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/index.js new file mode 100644 index 0000000000..7685a6d795 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/index.js @@ -0,0 +1 @@ +require("a") diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/package.json b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/package.json new file mode 100644 index 0000000000..99d3f2225b --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/package.json @@ -0,0 +1 @@ +{"dependencies":{"a":"file:./a", "d":"file:./d", "e":"file:./e"}} diff --git a/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/yarn.lock b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/yarn.lock new file mode 100644 index 0000000000..fd48530ef8 --- /dev/null +++ b/__tests__/fixtures/install/install-should-not-hoist-through-peer-deps/yarn.lock @@ -0,0 +1,28 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +"a@file:./a": + version "1.0.0" + dependencies: + b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-2" + c "file:../../../Users/mael/Library/Caches/Yarn/v1/c" + +"b@file:b-1": + version "1.0.0" + +"b@file:b-2": + version "2.0.0" + +"c@file:c": + version "1.0.0" + +"d@file:./d": + version "1.0.0" + dependencies: + b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1" + +"e@file:./e": + version "1.0.0" + dependencies: + b "file:../../../Users/mael/Library/Caches/Yarn/v1/b-1" diff --git a/src/package-hoister.js b/src/package-hoister.js index ac7c70db1c..e06f7969b0 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -121,7 +121,41 @@ export default class PackageHoister { return sortAlpha(aPattern, bPattern); }); - for (const [pattern, parent] of queue) { + // sort the queue again to hoist packages without peer dependencies first + let sortedQueue = []; + const availableSet = new Set(); + + let hasChanged = true; + while (queue.length > 0 && hasChanged) { + hasChanged = false; + + for (let t = 0; t < queue.length; ++t) { + const pattern = queue[t][0]; + const pkg = this.resolver.getStrictResolvedPattern(pattern); + + const peerDependencies = Object.keys(pkg.peerDependencies || {}); + const areDependenciesFulfilled = peerDependencies.every(peerDependency => availableSet.has(peerDependency)); + + if (areDependenciesFulfilled) { + // Move the package inside our sorted queue + sortedQueue.push(queue[t]); + queue.splice(t--, 1); + + // Add it to our set, so that we know it is available + availableSet.add(pattern); + + // Schedule a next pass, in case other packages had peer dependencies on this one + hasChanged = true; + } + } + } + + // We might end up with some packages left in the queue, that have not been sorted. We reach this codepath if two + // packages have a cyclic dependency, or if the peer dependency is provided by a parent package. In these case, + // nothing we can do, so we just add all of these packages to the end of the sorted queue. + sortedQueue = sortedQueue.concat(queue); + + for (const [pattern, parent] of sortedQueue) { const info = this._seed(pattern, {isDirectRequire: false, parent}); if (info) { this.hoist(info); @@ -259,13 +293,13 @@ export default class PackageHoister { const stack = []; // stack of removed parts const name = parts.pop(); - // for (let i = parts.length - 1; i >= 0; i--) { const checkParts = parts.slice(0, i).concat(name); const checkKey = this.implodeKey(checkParts); info.addHistory(`Looked at ${checkKey} for a match`); const existing = this.tree.get(checkKey); + if (existing) { if (existing.loc === info.loc) { // switch to non ignored if earlier deduped version was ignored (must be compatible) @@ -291,8 +325,24 @@ export default class PackageHoister { } } + const peerDependencies = Object.keys(info.pkg.peerDependencies || {}); + // remove redundant parts that wont collide - while (parts.length) { + hoistLoop: while (parts.length) { + // we must not hoist a package higher than its peer dependencies + for (const peerDependency of peerDependencies) { + const checkParts = parts.concat(peerDependency); + const checkKey = this.implodeKey(checkParts); + info.addHistory(`Looked at ${checkKey} for a peer dependency match`); + + const existing = this.tree.get(checkKey); + + if (existing) { + info.addHistory(`Found a peer dependency requirement at ${checkKey}`); + break hoistLoop; + } + } + const checkParts = parts.concat(name); const checkKey = this.implodeKey(checkParts);