Skip to content
This repository was archived by the owner on Jan 20, 2022. It is now read-only.

Commit bb2f9c0

Browse files
committed
Replace entire dep sets, so peer loops don't lock
Discovered an interesting issue with the following dependency set: ```json { "devDependencies": { "webpack-dev-server": "^3.11.0", "@pmmmwh/react-refresh-webpack-plugin": "^0.4.3" } } ``` `webpack-dev-server` here has a peerDependency on webpack v4. But, the dependencies of `@pmmmwh/react-refresh-webpack-plugin` have peer dependencies on webpack at `4 || 5`. So, a resolution _should_ be possible. Since the `@pmmmwh/react-refresh-webpack-plugin` package is alphabetically first, it loads its deps, and ends up placing webpack@5 to satisfy the dep with the latest and greatest. Then when `webpack-dev-server` gets to its eventual peer dep on webpack, it can't replace it, because `webpack@5` has a dependency on `terser-webpack-plugin@^5.0.3`, which in turn has a peer dependency on `[email protected]`. When we check to see if webpack 5 can be replaced, we find it has a dependent, and reject the replacement. But that dependent is only present as a dependency of the thing being replaced, so should not be considered. Second, while the source of the `terser-webpack-plugin` is `webpack`, it cannot be installed there, because it has peer deps that are also peer deps of webpack. So, we _must_ place it in the root node_modules, but were not attempting the "source" location overrides, because the root is not the source. This was a second issue resulting in `ERESOLVE` errors even when `--force` was applied, with this dependency set: ```json { "devDependencies": { "webpack-dev-server": "^3.11.0", "webpack": "^5.0.0" } } ``` Fixes: #180 Fixes: npm/cli#2123
1 parent 290a58a commit bb2f9c0

File tree

5 files changed

+30392
-15
lines changed

5 files changed

+30392
-15
lines changed

lib/arborist/build-ideal-tree.js

+33-14
Original file line numberDiff line numberDiff line change
@@ -1085,14 +1085,22 @@ This is a one-time fix-up, please be patient...
10851085

10861086
let target
10871087
let canPlace = null
1088+
let isSource = false
1089+
const source = this[_peerSetSource].get(dep)
10881090
for (let check = start; check; check = check.resolveParent) {
1091+
// we always give the FIRST place we possibly *can* put this a little
1092+
// extra prioritization with peer dep overrides and deduping
1093+
if (check === source)
1094+
isSource = true
1095+
10891096
// if the current location has a peerDep on it, then we can't place here
10901097
// this is pretty rare to hit, since we always prefer deduping peers.
10911098
const checkEdge = check.edgesOut.get(edge.name)
10921099
if (!check.isTop && checkEdge && checkEdge.peer)
10931100
continue
10941101

1095-
const cp = this[_canPlaceDep](dep, check, edge, peerEntryEdge, peerPath)
1102+
const cp = this[_canPlaceDep](dep, check, edge, peerEntryEdge, peerPath, isSource)
1103+
isSource = false
10961104

10971105
// anything other than a conflict is fine to proceed with
10981106
if (cp !== CONFLICT) {
@@ -1164,7 +1172,7 @@ This is a one-time fix-up, please be patient...
11641172
const oldDeps = []
11651173
for (const [name, edge] of oldChild.edgesOut.entries()) {
11661174
if (!newDep.edgesOut.has(name) && edge.to)
1167-
oldDeps.push(edge.to)
1175+
oldDeps.push(...gatherDepSet([edge.to], e => e.to !== edge.to))
11681176
}
11691177
newDep.replace(oldChild)
11701178
this[_pruneForReplacement](newDep, oldDeps)
@@ -1265,14 +1273,17 @@ This is a one-time fix-up, please be patient...
12651273
// deps that the new node doesn't depend on but the old one did.
12661274
const invalidDeps = new Set([...node.edgesOut.values()]
12671275
.filter(e => e.to && !e.valid).map(e => e.to))
1268-
for (const dep of oldDeps)
1269-
invalidDeps.add(dep)
1276+
for (const dep of oldDeps) {
1277+
const set = gatherDepSet([dep], e => e.to !== dep && e.valid)
1278+
for (const dep of set)
1279+
invalidDeps.add(dep)
1280+
}
12701281

12711282
// ignore dependency edges from the node being replaced, but
12721283
// otherwise filter the set down to just the set with no
12731284
// dependencies from outside the set, except the node in question.
12741285
const deps = gatherDepSet(invalidDeps, edge =>
1275-
edge.from !== node && edge.to !== node)
1286+
edge.from !== node && edge.to !== node && edge.valid)
12761287

12771288
// now just delete whatever's left, because it's junk
12781289
for (const dep of deps)
@@ -1299,16 +1310,24 @@ This is a one-time fix-up, please be patient...
12991310
// checking, because either we're leaving it alone, or it won't work anyway.
13001311
// When we check peers, we pass along the peerEntryEdge to track the
13011312
// original edge that caused us to load the family of peer dependencies.
1302-
[_canPlaceDep] (dep, target, edge, peerEntryEdge = null, peerPath = []) {
1313+
[_canPlaceDep] (dep, target, edge, peerEntryEdge = null, peerPath = [], isSource = false) {
13031314
/* istanbul ignore next */
13041315
debug(() => {
13051316
if (!dep)
13061317
throw new Error('no dep??')
13071318
})
13081319
const entryEdge = peerEntryEdge || edge
13091320
const source = this[_peerSetSource].get(dep)
1310-
const isSource = target === source
1311-
const { isRoot, isWorkspace } = source || {}
1321+
isSource = isSource || target === source
1322+
// if we're overriding the source, then we care if the *target* is
1323+
// ours, even if it wasn't actually the original source, since we
1324+
// are depending on something that has a dep that can't go in its own
1325+
// folder. for example, a -> b, b -> PEER(a). Even though a is the
1326+
// source, b has to be installed up a level, and if the root package
1327+
// depends on a, and it has a conflict, it's our problem. So, the root
1328+
// (or whatever is bringing in a) becomes the "effective source" for
1329+
// the purposes of this calculation.
1330+
const { isRoot, isWorkspace } = isSource ? target : source || {}
13121331
const isMine = isRoot || isWorkspace
13131332

13141333
// Useful testing thingie right here.
@@ -1333,7 +1352,7 @@ This is a one-time fix-up, please be patient...
13331352
const { version: newVer } = dep
13341353
const tryReplace = curVer && newVer && semver.gte(newVer, curVer)
13351354
if (tryReplace && dep.canReplace(current)) {
1336-
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
1355+
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
13371356
/* istanbul ignore else - It's extremely rare that a replaceable
13381357
* node would be a conflict, if the current one wasn't a conflict,
13391358
* but it is theoretically possible if peer deps are pinned. In
@@ -1353,7 +1372,7 @@ This is a one-time fix-up, please be patient...
13531372
// a bit harder to be singletons.
13541373
const preferDedupe = this[_preferDedupe] || edge.peer
13551374
if (preferDedupe && !tryReplace && dep.canReplace(current)) {
1356-
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
1375+
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
13571376
/* istanbul ignore else - It's extremely rare that a replaceable
13581377
* node would be a conflict, if the current one wasn't a conflict,
13591378
* but it is theoretically possible if peer deps are pinned. In
@@ -1421,7 +1440,7 @@ This is a one-time fix-up, please be patient...
14211440
}
14221441
}
14231442
if (canReplace) {
1424-
const ret = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
1443+
const ret = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
14251444
/* istanbul ignore else - extremely rare that the peer set would
14261445
* conflict if we can replace the node in question, but theoretically
14271446
* possible, if peer deps are pinned aggressively. */
@@ -1482,14 +1501,14 @@ This is a one-time fix-up, please be patient...
14821501
}
14831502

14841503
// no objections! ok to place here
1485-
return this[_canPlacePeers](dep, target, edge, OK, peerEntryEdge, peerPath)
1504+
return this[_canPlacePeers](dep, target, edge, OK, peerEntryEdge, peerPath, isSource)
14861505
}
14871506

14881507
// make sure the family of peer deps can live here alongside it.
14891508
// this doesn't guarantee that THIS solution will be the one we take,
14901509
// but it does establish that SOME solution exists at this level in
14911510
// the tree.
1492-
[_canPlacePeers] (dep, target, edge, ret, peerEntryEdge, peerPath) {
1511+
[_canPlacePeers] (dep, target, edge, ret, peerEntryEdge, peerPath, isSource) {
14931512
// do not go in cycles when we're resolving a peer group
14941513
if (!dep.parent || peerEntryEdge && peerPath.includes(dep))
14951514
return ret
@@ -1501,7 +1520,7 @@ This is a one-time fix-up, please be patient...
15011520
if (!peerEdge.peer || !peerEdge.to)
15021521
continue
15031522
const peer = peerEdge.to
1504-
const canPlacePeer = this[_canPlaceDep](peer, target, peerEdge, entryEdge, peerPath)
1523+
const canPlacePeer = this[_canPlaceDep](peer, target, peerEdge, entryEdge, peerPath, isSource)
15051524
if (canPlacePeer !== CONFLICT)
15061525
continue
15071526

lib/node.js

+8-1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ const {normalize} = require('read-package-json-fast')
3535
const {getPaths: getBinPaths} = require('bin-links')
3636
const npa = require('npm-package-arg')
3737
const debug = require('./debug.js')
38+
const gatherDepSet = require('./gather-dep-set.js')
3839

3940
const {resolve, relative, dirname, basename} = require('path')
4041
const _package = Symbol('_package')
@@ -640,8 +641,14 @@ class Node {
640641
if (node.name !== this.name)
641642
return false
642643

644+
// gather up all the deps of this node and that are only depended
645+
// upon by deps of this node. those ones don't count, since
646+
// they'll be replaced if this node is replaced anyway.
647+
const depSet = gatherDepSet([this], e => e.to !== this && e.valid)
648+
643649
for (const edge of this.edgesIn) {
644-
if (!edge.satisfiedBy(node))
650+
// only care about edges that don't originate from this node
651+
if (!depSet.has(edge.from) && !edge.satisfiedBy(node))
645652
return false
646653
}
647654

scripts/reify.js

+6
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@ process.on('timeEnd', name => {
1515
}
1616
const res = process.hrtime(timers[name])
1717
delete timers[name]
18+
19+
if (options.quiet)
20+
return
21+
1822
console.error(name, res[0] * 1e3 + res[1] / 1e6)
1923
})
2024
process.on('exit', () => {
@@ -104,6 +108,8 @@ process.on('log', (level, ...args) => {
104108
if (level === 'warn' && args[0] === 'ERESOLVE') {
105109
args[2] = require('util').inspect(args[2], { depth: Infinity })
106110
}
111+
if (options.quiet)
112+
return
107113
return console.error(level, ...args)
108114
})
109115

0 commit comments

Comments
 (0)