From d1977519d5afd18c9e721c0cd5b4d55d90b8f07d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Thu, 3 Aug 2017 15:45:55 +0100 Subject: [PATCH 1/4] Prevents hoisting through peer dependencies --- __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 | 19 ++++++++++++- 13 files changed, 62 insertions(+), 1 deletion(-) 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 d9ece3a6ec..904f3ea35c 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..fe1dded4ba 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -259,13 +259,15 @@ export default class PackageHoister { const stack = []; // stack of removed parts const name = parts.pop(); - // + const peerDependencies = Object.keys(info.pkg.peerDependencies || {}); + 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) @@ -293,6 +295,21 @@ export default class PackageHoister { // remove redundant parts that wont collide 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}`); + stepUp = true; + break; + } + } + const checkParts = parts.concat(name); const checkKey = this.implodeKey(checkParts); From 8d6772d4b2f9beb3614434ae2dcfa21087757334 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Thu, 3 Aug 2017 18:45:57 +0100 Subject: [PATCH 2/4] Ensures that packages without peer dependencies are hoisted before those that need it --- src/package-hoister.js | 46 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/src/package-hoister.js b/src/package-hoister.js index fe1dded4ba..a25227f380 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -121,7 +121,42 @@ 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(); + + for (let hasChanged = true; 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,8 +294,6 @@ export default class PackageHoister { const stack = []; // stack of removed parts const name = parts.pop(); - const peerDependencies = Object.keys(info.pkg.peerDependencies || {}); - for (let i = parts.length - 1; i >= 0; i--) { const checkParts = parts.slice(0, i).concat(name); const checkKey = this.implodeKey(checkParts); @@ -293,8 +326,10 @@ 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); @@ -305,8 +340,7 @@ export default class PackageHoister { if (existing) { info.addHistory(`Found a peer dependency requirement at ${checkKey}`); - stepUp = true; - break; + break hoistLoop; } } From ce47a77eccb0c60c9252f273d411b0cfb179bfa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 4 Aug 2017 14:37:16 +0100 Subject: [PATCH 3/4] Uses while instead of weird-for --- src/package-hoister.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/package-hoister.js b/src/package-hoister.js index a25227f380..b415267ee8 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -125,7 +125,8 @@ export default class PackageHoister { let sortedQueue = []; const availableSet = new Set(); - for (let hasChanged = true; queue.length > 0 && hasChanged;) { + let hasChanged = true;\ + while (queue.length > 0 && hasChanged) { hasChanged = false; for (let t = 0; t < queue.length; ++t) { From 8119408837bacf17d4c9dc086d8c682ca0ff9246 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 4 Aug 2017 15:02:03 +0100 Subject: [PATCH 4/4] Linting --- __tests__/commands/install/integration.js | 2 +- src/package-hoister.js | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index 904f3ea35c..ebed3efe3c 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -51,7 +51,7 @@ 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); + expect(await fs.exists(`${config.cwd}/node_modules/a/node_modules/c`)).toEqual(true); }); }); diff --git a/src/package-hoister.js b/src/package-hoister.js index b415267ee8..e06f7969b0 100644 --- a/src/package-hoister.js +++ b/src/package-hoister.js @@ -125,7 +125,7 @@ export default class PackageHoister { let sortedQueue = []; const availableSet = new Set(); - let hasChanged = true;\ + let hasChanged = true; while (queue.length > 0 && hasChanged) { hasChanged = false; @@ -137,7 +137,6 @@ export default class PackageHoister { 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); @@ -147,14 +146,13 @@ export default class PackageHoister { // 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. + // 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) {