Skip to content

Commit 88cc4fc

Browse files
sokraaeschright
sokra
authored and
aeschright
committed
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
1 parent 75dce4c commit 88cc4fc

4 files changed

Lines changed: 209 additions & 10 deletions

File tree

‎lib/dedupe.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,9 @@ function hoistChildren_ (tree, diff, seen, next) {
142142
[andComputeMetadata(tree)]
143143
], done)
144144
}
145-
var hoistTo = earliestInstallable(tree, tree.parent, child.package, log)
146-
if (hoistTo) {
145+
// find a location where it's installable
146+
var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
147+
if (hoistTo && hoistTo !== tree) {
147148
move(child, hoistTo, diff)
148149
chain([
149150
[andComputeMetadata(hoistTo)],

‎lib/install/deps.js‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,7 @@ function resolveWithNewModule (pkg, tree, log, next) {
650650
return isInstallable(pkg, (err) => {
651651
let installable = !err
652652
addBundled(pkg, (bundleErr) => {
653-
var parent = earliestInstallable(tree, tree, pkg, log) || tree
653+
var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
654654
var isLink = pkg._requested.type === 'directory'
655655
var child = createChild({
656656
package: pkg,
@@ -755,11 +755,11 @@ function preserveSymlinks () {
755755
// Find the highest level in the tree that we can install this module in.
756756
// If the module isn't installed above us yet, that'd be the very top.
757757
// If it is, then it's the level below where its installed.
758-
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log) {
759-
validate('OOOO', arguments)
758+
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
759+
validate('OOOOZ|OOOOO', arguments)
760760

761761
function undeletedModuleMatches (child) {
762-
return !child.removed && moduleName(child) === pkg.name
762+
return child !== ignoreChild && !child.removed && moduleName(child) === pkg.name
763763
}
764764
const undeletedMatches = tree.children.filter(undeletedModuleMatches)
765765
if (undeletedMatches.length) {
@@ -778,7 +778,7 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr
778778
// If any of the children of this tree have conflicting
779779
// binaries then we need to decline to install this package here.
780780
var binaryMatches = pkg.bin && tree.children.some(function (child) {
781-
if (child.removed || !child.package.bin) return false
781+
if (child === ignoreChild || child.removed || !child.package.bin) return false
782782
return Object.keys(child.package.bin).some(function (bin) {
783783
return pkg.bin[bin]
784784
})
@@ -804,6 +804,23 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr
804804

805805
if (tree.phantomChildren && tree.phantomChildren[pkg.name]) return null
806806

807+
// if any of the peer dependencies is a dependency of the current
808+
// tree locations, we place the package here. This is a safe location
809+
// where we don't violate the peer dependencies contract.
810+
// It may not be the perfect location in some cases, but we don't know
811+
// if dependencies are hoisted to another location yet, as they
812+
// may not be loaded into the tree yet.
813+
// We can ignore dev deps here as they are only installed on top-level
814+
// and we can't hoist further than that anyway.
815+
var peerDeps = pkg.peerDependencies
816+
if (peerDeps) {
817+
if (Object.keys(peerDeps).some(function (name) {
818+
return deps[name]
819+
})) {
820+
return tree
821+
}
822+
}
823+
807824
if (tree.isTop) return tree
808825
if (tree.isGlobal) return tree
809826

@@ -812,5 +829,5 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr
812829

813830
if (!preserveSymlinks() && /^[.][.][\\/]/.test(path.relative(tree.parent.realpath, tree.realpath))) return tree
814831

815-
return (earliestInstallable(requiredBy, tree.parent, pkg, log) || tree)
832+
return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
816833
}

‎test/tap/hoist-peer-dependencies.js‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
'use strict'
2+
const path = require('path')
3+
const fs = require('fs')
4+
const test = require('tap').test
5+
const Tacks = require('tacks')
6+
const File = Tacks.File
7+
const Dir = Tacks.Dir
8+
const common = require('../common-tap.js')
9+
10+
const basedir = path.join(__dirname, path.basename(__filename, '.js'))
11+
const testdir = path.join(basedir, 'testdir')
12+
const cachedir = path.join(basedir, 'cache')
13+
const globaldir = path.join(basedir, 'global')
14+
const tmpdir = path.join(basedir, 'tmp')
15+
16+
const conf = {
17+
cwd: testdir,
18+
env: common.newEnv().extend({
19+
npm_config_cache: cachedir,
20+
npm_config_tmp: tmpdir,
21+
npm_config_prefix: globaldir,
22+
npm_config_registry: common.registry,
23+
npm_config_loglevel: 'warn'
24+
})
25+
}
26+
27+
const fixture = new Tacks(Dir({
28+
cache: Dir(),
29+
global: Dir(),
30+
tmp: Dir(),
31+
testdir: Dir({
32+
package: Dir({
33+
'package.json': File({
34+
name: 'package',
35+
version: '1.0.0',
36+
dependencies: {
37+
'base-dep': '../packages/base-dep-1.0.0.tgz',
38+
'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
39+
}
40+
})
41+
}),
42+
'package.json': File({
43+
version: '1.0.0',
44+
dependencies: {
45+
package: 'file:./package',
46+
'base-dep': './packages/base-dep-2.0.0.tgz'
47+
}
48+
}),
49+
// file: dependencies do not work as they are symlinked and behave
50+
// differently. Instead installation from tgz files is used.
51+
packages: Dir({
52+
'base-dep-1.0.0.tgz': File(Buffer.from(
53+
'1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
54+
'06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
55+
'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
56+
'4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
57+
'0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
58+
'hex'
59+
)),
60+
'base-dep-2.0.0.tgz': File(Buffer.from(
61+
'1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
62+
'06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
63+
'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
64+
'4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
65+
'0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
66+
'hex'
67+
)),
68+
'plugin-dep-1.0.0.tgz': File(Buffer.from(
69+
'1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
70+
'295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
71+
'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
72+
'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
73+
'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
74+
'3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
75+
'2e00080000',
76+
'hex'
77+
))
78+
})
79+
})
80+
}))
81+
82+
function setup () {
83+
cleanup()
84+
fixture.create(basedir)
85+
}
86+
87+
function cleanup () {
88+
fixture.remove(basedir)
89+
}
90+
91+
test('setup', t => {
92+
setup()
93+
return common.fakeRegistry.listen()
94+
})
95+
96+
test('example', t => {
97+
return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
98+
t.is(code, 0, 'command ran ok')
99+
t.comment(stdout.trim())
100+
t.comment(stderr.trim())
101+
t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
102+
var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
103+
t.similar(packageLock, {
104+
dependencies: {
105+
'base-dep': {
106+
version: 'file:packages/base-dep-2.0.0.tgz'
107+
},
108+
'package': {
109+
version: 'file:package',
110+
dependencies: {
111+
'base-dep': {
112+
version: 'file:packages/base-dep-1.0.0.tgz'
113+
},
114+
// plugin-dep must not placed on top-level
115+
'plugin-dep': {
116+
version: 'file:packages/plugin-dep-1.0.0.tgz'
117+
}
118+
}
119+
}
120+
}
121+
}, 'locks dependencies as unhoisted')
122+
t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
123+
})
124+
})
125+
126+
test('cleanup', t => {
127+
common.fakeRegistry.close()
128+
cleanup()
129+
t.done()
130+
})

‎test/tap/unit-deps-earliestInstallable.js‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ test('earliestInstallable should consider devDependencies', function (t) {
6767
dep2a.parent = dep1
6868
dep2.parent = pkg
6969

70-
var earliest = earliestInstallable(dep1, dep1, dep2a.package, log)
70+
var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
7171
t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
7272
t.end()
7373
})
@@ -108,7 +108,58 @@ test('earliestInstallable should reuse shared prod/dev deps when they are identi
108108
dep1.parent = pkg
109109
dep2.parent = pkg
110110

111-
var earliest = earliestInstallable(dep1, dep1, dep2.package, log)
111+
var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
112112
t.isDeeply(earliest, pkg, 'should reuse identical shared dev/prod deps when installing both')
113113
t.end()
114114
})
115+
116+
test('earliestInstallable should consider peerDependencies', function (t) {
117+
var dep1 = {
118+
children: [],
119+
package: {
120+
name: 'dep1',
121+
dependencies: {
122+
dep2: '1.0.0',
123+
dep3: '1.0.0'
124+
}
125+
},
126+
path: '/dep1',
127+
realpath: '/dep1'
128+
}
129+
130+
var dep2 = {
131+
package: {
132+
name: 'dep2',
133+
version: '1.0.0',
134+
peerDependencies: {
135+
dep3: '1.0.0'
136+
},
137+
_requested: npa('dep2@^1.0.0')
138+
},
139+
parent: dep1,
140+
path: '/dep1/node_modules/dep2',
141+
realpath: '/dep1/node_modules/dep2'
142+
}
143+
144+
var pkg = {
145+
isTop: true,
146+
children: [dep1],
147+
package: {
148+
name: 'pkg',
149+
dependencies: { dep1: '1.0.0' }
150+
},
151+
path: '/',
152+
realpath: '/'
153+
}
154+
155+
dep1.parent = pkg
156+
157+
var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
158+
t.isDeeply(earliest, dep1, 'should not be able to hoist the package to top-level')
159+
160+
dep1.children.push(dep2)
161+
162+
var earliestWithIgnore = earliestInstallable(dep1, dep1, dep2.package, log, dep2)
163+
t.isDeeply(earliestWithIgnore, dep1, 'should not be able to hoist the package to top-level')
164+
t.end()
165+
})

0 commit comments

Comments
 (0)
⚡