Skip to content

Commit

Permalink
install/dedupe: fix hoisting of packages with peerDeps (#147)
Browse files Browse the repository at this point in the history
  • Loading branch information
sokra authored and aeschright committed Jan 29, 2019
1 parent cdb0592 commit 91314e7
Show file tree
Hide file tree
Showing 4 changed files with 209 additions and 10 deletions.
5 changes: 3 additions & 2 deletions lib/dedupe.js
Expand Up @@ -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)],
Expand Down
29 changes: 23 additions & 6 deletions lib/install/deps.js
Expand Up @@ -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,
Expand Down Expand Up @@ -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) {
Expand All @@ -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]
})
Expand All @@ -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

Expand All @@ -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)
}
130 changes: 130 additions & 0 deletions 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()
})
55 changes: 53 additions & 2 deletions test/tap/unit-deps-earliestInstallable.js
Expand Up @@ -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()
})
Expand Down Expand Up @@ -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()
})

0 comments on commit 91314e7

Please sign in to comment.