install/dedupe: fix hoisting of packages with peerDeps (#147)

PR-URL: #147
Fixes: https://npm.community/t/packages-with-peerdependencies-are-incorrectly-hoisted/4794

Credit: @sokra
Reviewed-By: @aeschright

Author: sokra

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)],

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)
 }

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()
+})

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()
+})