diff --git a/lib/dedupe.js b/lib/dedupe.js index 325faeaabcd43..fae409fea572d 100644 --- a/lib/dedupe.js +++ b/lib/dedupe.js @@ -142,8 +142,9 @@ 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 + var hoistTo = earliestInstallable(tree, tree, child.package, log, child) + 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..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] }) @@ -804,6 +804,23 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr if (tree.phantomChildren && tree.phantomChildren[pkg.name]) return null + // 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 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] + })) { + return tree + } + } + if (tree.isTop) return tree if (tree.isGlobal) return tree @@ -812,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/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()) diff --git a/test/tap/hoist-peer-dependencies.js b/test/tap/hoist-peer-dependencies.js new file mode 100644 index 0000000000000..75d7d3e778ada --- /dev/null +++ b/test/tap/hoist-peer-dependencies.js @@ -0,0 +1,130 @@ +'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(Buffer.from( + '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + + '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' + + '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000', + 'hex' + )), + 'base-dep-2.0.0.tgz': File(Buffer.from( + '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' + + '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' + + 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' + + '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' + + '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000', + 'hex' + )), + 'plugin-dep-1.0.0.tgz': File(Buffer.from( + '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() +}) diff --git a/test/tap/unit-deps-earliestInstallable.js b/test/tap/unit-deps-earliestInstallable.js index 47d1ab4119b1e..136e6478c586a 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() +})