From 4bd5a3ef7e72bdcbdf23ef3a563d4a6894ca23d4 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Thu, 24 Jan 2019 09:49:54 +0100 Subject: [PATCH 1/8] install/dedupe: fix hoisting of packages with peerDeps --- lib/dedupe.js | 8 ++++++-- lib/install/deps.js | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/dedupe.js b/lib/dedupe.js index 325faeaabcd43..62975d20335de 100644 --- a/lib/dedupe.js +++ b/lib/dedupe.js @@ -142,8 +142,12 @@ function hoistChildren_ (tree, diff, seen, next) { [andComputeMetadata(tree)] ], done) } - var hoistTo = earliestInstallable(tree, tree.parent, child.package, log) - if (hoistTo) { + // find a location where it's installable + // child is marked as removed so it's ignored when finding a location + child.removed = true + var hoistTo = earliestInstallable(tree, tree, child.package, log) + child.removed = false + if (hoistTo && hoistTo !== tree) { move(child, hoistTo, diff) chain([ [andComputeMetadata(hoistTo)], diff --git a/lib/install/deps.js b/lib/install/deps.js index c36265093b090..ae4aff9b49273 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -804,6 +804,21 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr if (tree.phantomChildren && tree.phantomChildren[pkg.name]) return null + // if any of the peer dependencies is a (dev) dependency of the current + // tree locations, we place the package here. This is a safe location + // where we don't violate the peer dependencies contract. + // It may not be the perfect location in same cases, but we don't know + // if (dev) dependencies are hoisted to another location yet, as they + // may not be loaded into the tree yet. + var peerDeps = pkg.peerDependencies + if (peerDeps) { + if (Object.keys(peerDeps).some(function (name) { + return deps[name] || devDeps[name] + })) { + return tree + } + } + if (tree.isTop) return tree if (tree.isGlobal) return tree From 214f81ee8c38a2f85a953c4c2ec352fdeb349cdc Mon Sep 17 00:00:00 2001 From: Kinrany Date: Thu, 24 Jan 2019 19:06:31 +0100 Subject: [PATCH 2/8] Fix typo Co-Authored-By: sokra --- lib/install/deps.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/install/deps.js b/lib/install/deps.js index ae4aff9b49273..12caff57ad7ef 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -807,7 +807,7 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr // if any of the peer dependencies is a (dev) dependency of the current // tree locations, we place the package here. This is a safe location // where we don't violate the peer dependencies contract. - // It may not be the perfect location in same cases, but we don't know + // It may not be the perfect location in some cases, but we don't know // if (dev) dependencies are hoisted to another location yet, as they // may not be loaded into the tree yet. var peerDeps = pkg.peerDependencies From 85b0c5db993ea1570f522500f74a50dd20212d13 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sun, 27 Jan 2019 11:57:14 +0100 Subject: [PATCH 3/8] ignore devDeps when checking peerDeps --- lib/install/deps.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/install/deps.js b/lib/install/deps.js index 12caff57ad7ef..53102851f85ff 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -804,16 +804,18 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr if (tree.phantomChildren && tree.phantomChildren[pkg.name]) return null - // if any of the peer dependencies is a (dev) dependency of the current + // if any of the peer dependencies is a dependency of the current // tree locations, we place the package here. This is a safe location // where we don't violate the peer dependencies contract. // It may not be the perfect location in some cases, but we don't know - // if (dev) dependencies are hoisted to another location yet, as they + // if dependencies are hoisted to another location yet, as they // may not be loaded into the tree yet. + // We can ignore dev deps here as they are only installed on top-level + // and we can't hoist further than that anyway. var peerDeps = pkg.peerDependencies if (peerDeps) { if (Object.keys(peerDeps).some(function (name) { - return deps[name] || devDeps[name] + return deps[name] })) { return tree } From 7dbc8ccdf7a78c5a57701c95cdb1a72cf5ddeecb Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sun, 27 Jan 2019 12:14:24 +0100 Subject: [PATCH 4/8] add ignoreChild argument to earliestInstallable --- lib/dedupe.js | 4 +- lib/install/deps.js | 12 ++--- test/tap/unit-deps-earliestInstallable.js | 55 ++++++++++++++++++++++- 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/lib/dedupe.js b/lib/dedupe.js index 62975d20335de..d8d5260ea2e06 100644 --- a/lib/dedupe.js +++ b/lib/dedupe.js @@ -144,9 +144,7 @@ function hoistChildren_ (tree, diff, seen, next) { } // find a location where it's installable // child is marked as removed so it's ignored when finding a location - child.removed = true - var hoistTo = earliestInstallable(tree, tree, child.package, log) - child.removed = false + var hoistTo = earliestInstallable(tree, tree, child.package, log, child) if (hoistTo && hoistTo !== tree) { move(child, hoistTo, diff) chain([ diff --git a/lib/install/deps.js b/lib/install/deps.js index 53102851f85ff..1cf4de15dc3f1 100644 --- a/lib/install/deps.js +++ b/lib/install/deps.js @@ -650,7 +650,7 @@ function resolveWithNewModule (pkg, tree, log, next) { return isInstallable(pkg, (err) => { let installable = !err addBundled(pkg, (bundleErr) => { - var parent = earliestInstallable(tree, tree, pkg, log) || tree + var parent = earliestInstallable(tree, tree, pkg, log, null) || tree var isLink = pkg._requested.type === 'directory' var child = createChild({ package: pkg, @@ -755,11 +755,11 @@ function preserveSymlinks () { // Find the highest level in the tree that we can install this module in. // If the module isn't installed above us yet, that'd be the very top. // If it is, then it's the level below where its installed. -var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log) { - validate('OOOO', arguments) +var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) { + validate('OOOOZ|OOOOO', arguments) function undeletedModuleMatches (child) { - return !child.removed && moduleName(child) === pkg.name + return child !== ignoreChild && !child.removed && moduleName(child) === pkg.name } const undeletedMatches = tree.children.filter(undeletedModuleMatches) if (undeletedMatches.length) { @@ -778,7 +778,7 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr // If any of the children of this tree have conflicting // binaries then we need to decline to install this package here. var binaryMatches = pkg.bin && tree.children.some(function (child) { - if (child.removed || !child.package.bin) return false + if (child === ignoreChild || child.removed || !child.package.bin) return false return Object.keys(child.package.bin).some(function (bin) { return pkg.bin[bin] }) @@ -829,5 +829,5 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr if (!preserveSymlinks() && /^[.][.][\\/]/.test(path.relative(tree.parent.realpath, tree.realpath))) return tree - return (earliestInstallable(requiredBy, tree.parent, pkg, log) || tree) + return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree) } diff --git a/test/tap/unit-deps-earliestInstallable.js b/test/tap/unit-deps-earliestInstallable.js index 47d1ab4119b1e..e188c3610eba6 100644 --- a/test/tap/unit-deps-earliestInstallable.js +++ b/test/tap/unit-deps-earliestInstallable.js @@ -67,7 +67,7 @@ test('earliestInstallable should consider devDependencies', function (t) { dep2a.parent = dep1 dep2.parent = pkg - var earliest = earliestInstallable(dep1, dep1, dep2a.package, log) + var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null) t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present') t.end() }) @@ -108,7 +108,58 @@ test('earliestInstallable should reuse shared prod/dev deps when they are identi dep1.parent = pkg dep2.parent = pkg - var earliest = earliestInstallable(dep1, dep1, dep2.package, log) + var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null) t.isDeeply(earliest, pkg, 'should reuse identical shared dev/prod deps when installing both') t.end() }) + +test('earliestInstallable should consider peerDependencies', function (t) { + var dep1 = { + children: [], + package: { + name: 'dep1', + dependencies: { + dep2: '1.0.0', + dep3: '1.0.0' + } + }, + path: '/dep1', + realpath: '/dep1' + } + + var dep2 = { + package: { + name: 'dep2', + version: '1.0.0', + peerDependencies: { + dep3: '1.0.0' + }, + _requested: npa('dep2@^1.0.0') + }, + parent: dep1, + path: '/dep1/node_modules/dep2', + realpath: '/dep1/node_modules/dep2' + } + + var pkg = { + isTop: true, + children: [dep1], + package: { + name: 'pkg', + dependencies: { dep1: '1.0.0' }, + }, + path: '/', + realpath: '/' + } + + dep1.parent = pkg + + var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null) + t.isDeeply(earliest, dep1, 'should not be able to hoist the package to top-level') + + dep1.children.push(dep2) + + var earliestWithIgnore = earliestInstallable(dep1, dep1, dep2.package, log, dep2) + t.isDeeply(earliestWithIgnore, dep1, 'should not be able to hoist the package to top-level') + t.end() +}) From 27b02794cc8235d27a8bd2cfcf6073b056457858 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sun, 27 Jan 2019 13:02:04 +0100 Subject: [PATCH 5/8] add integration test for peerDependencies --- test/tap/hoist-peer-dependencies.js | 131 ++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 test/tap/hoist-peer-dependencies.js diff --git a/test/tap/hoist-peer-dependencies.js b/test/tap/hoist-peer-dependencies.js new file mode 100644 index 0000000000000..c50ad66cc14df --- /dev/null +++ b/test/tap/hoist-peer-dependencies.js @@ -0,0 +1,131 @@ +'use strict' +const path = require('path') +const fs = require('fs') +const test = require('tap').test +const Tacks = require('tacks') +const File = Tacks.File +const Dir = Tacks.Dir +const common = require('../common-tap.js') + +const basedir = path.join(__dirname, path.basename(__filename, '.js')) +const testdir = path.join(basedir, 'testdir') +const cachedir = path.join(basedir, 'cache') +const globaldir = path.join(basedir, 'global') +const tmpdir = path.join(basedir, 'tmp') + +const conf = { + cwd: testdir, + env: common.newEnv().extend({ + npm_config_cache: cachedir, + npm_config_tmp: tmpdir, + npm_config_prefix: globaldir, + npm_config_registry: common.registry, + npm_config_loglevel: 'warn' + }) +} + +const fixture = new Tacks(Dir({ + cache: Dir(), + global: Dir(), + tmp: Dir(), + testdir: Dir({ + package: Dir({ + 'package.json': File({ + name: 'package', + version: '1.0.0', + dependencies: { + 'base-dep': '../packages/base-dep-1.0.0.tgz', + 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz' + } + }) + }), + 'package.json': File({ + version: '1.0.0', + dependencies: { + package: 'file:./package', + 'base-dep': './packages/base-dep-2.0.0.tgz' + } + }), + // file: dependencies do not work as they are symlinked and behave + // differently. Instead installation from tgz files is used. + packages: Dir({ + 'base-dep-1.0.0.tgz': File(new Buffer( + '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + + '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' + + '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000', + 'hex' + )), + 'base-dep-2.0.0.tgz': File(new Buffer( + '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + + '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' + + '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000', + 'hex' + )), + 'plugin-dep-1.0.0.tgz': File(new Buffer( + '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' + + '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' + + 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' + + 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' + + 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' + + '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' + + '2e00080000', + 'hex' + )) + }) + }) +})) + +function setup () { + cleanup() + fixture.create(basedir) +} + +function cleanup () { + fixture.remove(basedir) +} + +test('setup', t => { + setup() + return common.fakeRegistry.listen() +}) + +test('example', t => { + return common.npm(['install'], conf).then(([code, stdout, stderr]) => { + t.is(code, 0, 'command ran ok') + t.comment(stdout.trim()) + t.comment(stderr.trim()) + t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install') + var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8')) + t.similar(packageLock, { + dependencies: { + 'base-dep': { + version: 'file:packages/base-dep-2.0.0.tgz', + }, + 'package': { + version: 'file:package', + dependencies: { + 'base-dep': { + version: 'file:packages/base-dep-1.0.0.tgz' + }, + // plugin-dep must not placed on top-level + 'plugin-dep': { + version: 'file:packages/plugin-dep-1.0.0.tgz' + } + } + } + } + }, 'locks dependencies as unhoisted') + t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level') + }) +}) + +test('cleanup', t => { + common.fakeRegistry.close() + cleanup() + t.done() +}) + From 9b23002532416d95e89426a14b698a956221c735 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sun, 27 Jan 2019 13:02:14 +0100 Subject: [PATCH 6/8] fix maketest script --- scripts/maketest | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/maketest b/scripts/maketest index bf0c2c5f655db..4ef1e9266fb04 100755 --- a/scripts/maketest +++ b/scripts/maketest @@ -69,7 +69,7 @@ test('setup', t => { }) test('example', t => { - return common.npm(['install'], conf).then((code, stdout, stderr) => { + return common.npm(['install'], conf).then(([code, stdout, stderr]) => { t.is(code, 0, 'command ran ok') t.comment(stdout.trim()) t.comment(stderr.trim()) From 0f0f2b46088a300ac30d56b7cec754763ed39d78 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Sun, 27 Jan 2019 13:04:52 +0100 Subject: [PATCH 7/8] remove invalid comment --- lib/dedupe.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/dedupe.js b/lib/dedupe.js index d8d5260ea2e06..fae409fea572d 100644 --- a/lib/dedupe.js +++ b/lib/dedupe.js @@ -143,7 +143,6 @@ function hoistChildren_ (tree, diff, seen, next) { ], done) } // find a location where it's installable - // child is marked as removed so it's ignored when finding a location var hoistTo = earliestInstallable(tree, tree, child.package, log, child) if (hoistTo && hoistTo !== tree) { move(child, hoistTo, diff) From 44953faecfac8636d6ca006136ba3953e93ec3e5 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Mon, 28 Jan 2019 08:33:26 +0100 Subject: [PATCH 8/8] fix linting issues --- test/tap/hoist-peer-dependencies.js | 9 ++++----- test/tap/unit-deps-earliestInstallable.js | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/test/tap/hoist-peer-dependencies.js b/test/tap/hoist-peer-dependencies.js index c50ad66cc14df..75d7d3e778ada 100644 --- a/test/tap/hoist-peer-dependencies.js +++ b/test/tap/hoist-peer-dependencies.js @@ -49,7 +49,7 @@ const fixture = new Tacks(Dir({ // file: dependencies do not work as they are symlinked and behave // differently. Instead installation from tgz files is used. packages: Dir({ - 'base-dep-1.0.0.tgz': File(new Buffer( + 'base-dep-1.0.0.tgz': File(Buffer.from( '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + @@ -57,7 +57,7 @@ const fixture = new Tacks(Dir({ '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000', 'hex' )), - 'base-dep-2.0.0.tgz': File(new Buffer( + 'base-dep-2.0.0.tgz': File(Buffer.from( '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + @@ -65,7 +65,7 @@ const fixture = new Tacks(Dir({ '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000', 'hex' )), - 'plugin-dep-1.0.0.tgz': File(new Buffer( + 'plugin-dep-1.0.0.tgz': File(Buffer.from( '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' + '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' + 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' + @@ -103,7 +103,7 @@ test('example', t => { t.similar(packageLock, { dependencies: { 'base-dep': { - version: 'file:packages/base-dep-2.0.0.tgz', + version: 'file:packages/base-dep-2.0.0.tgz' }, 'package': { version: 'file:package', @@ -128,4 +128,3 @@ test('cleanup', t => { cleanup() t.done() }) - diff --git a/test/tap/unit-deps-earliestInstallable.js b/test/tap/unit-deps-earliestInstallable.js index e188c3610eba6..136e6478c586a 100644 --- a/test/tap/unit-deps-earliestInstallable.js +++ b/test/tap/unit-deps-earliestInstallable.js @@ -146,7 +146,7 @@ test('earliestInstallable should consider peerDependencies', function (t) { children: [dep1], package: { name: 'pkg', - dependencies: { dep1: '1.0.0' }, + dependencies: { dep1: '1.0.0' } }, path: '/', realpath: '/'