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

Commit

Permalink
Replace entire dep sets, so peer loops don't lock
Browse files Browse the repository at this point in the history
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 `webpack@5.4.0`.

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.

Fixes: #180
Fixes: npm/cli#2123
  • Loading branch information
isaacs committed Nov 12, 2020
1 parent 290a58a commit 3eae280
Show file tree
Hide file tree
Showing 5 changed files with 30,392 additions and 15 deletions.
47 changes: 33 additions & 14 deletions lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -1085,14 +1085,22 @@ This is a one-time fix-up, please be patient...

let target
let canPlace = null
let isSource = false
const source = this[_peerSetSource].get(dep)
for (let check = start; check; check = check.resolveParent) {
// we always give the FIRST place we possibly *can* put this a little
// extra prioritization with peer dep overrides and deduping
if (check === source)
isSource = true

// if the current location has a peerDep on it, then we can't place here
// this is pretty rare to hit, since we always prefer deduping peers.
const checkEdge = check.edgesOut.get(edge.name)
if (!check.isTop && checkEdge && checkEdge.peer)
continue

const cp = this[_canPlaceDep](dep, check, edge, peerEntryEdge, peerPath)
const cp = this[_canPlaceDep](dep, check, edge, peerEntryEdge, peerPath, isSource)
isSource = false

// anything other than a conflict is fine to proceed with
if (cp !== CONFLICT) {
Expand Down Expand Up @@ -1164,7 +1172,7 @@ This is a one-time fix-up, please be patient...
const oldDeps = []
for (const [name, edge] of oldChild.edgesOut.entries()) {
if (!newDep.edgesOut.has(name) && edge.to)
oldDeps.push(edge.to)
oldDeps.push(...gatherDepSet([edge.to], e => e.to !== edge.to))
}
newDep.replace(oldChild)
this[_pruneForReplacement](newDep, oldDeps)
Expand Down Expand Up @@ -1265,14 +1273,17 @@ This is a one-time fix-up, please be patient...
// deps that the new node doesn't depend on but the old one did.
const invalidDeps = new Set([...node.edgesOut.values()]
.filter(e => e.to && !e.valid).map(e => e.to))
for (const dep of oldDeps)
invalidDeps.add(dep)
for (const dep of oldDeps) {
const set = gatherDepSet([dep], e => e.to !== dep && e.valid)
for (const dep of set)
invalidDeps.add(dep)
}

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

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

// Useful testing thingie right here.
Expand All @@ -1333,7 +1352,7 @@ This is a one-time fix-up, please be patient...
const { version: newVer } = dep
const tryReplace = curVer && newVer && semver.gte(newVer, curVer)
if (tryReplace && dep.canReplace(current)) {
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
/* istanbul ignore else - It's extremely rare that a replaceable
* node would be a conflict, if the current one wasn't a conflict,
* but it is theoretically possible if peer deps are pinned. In
Expand All @@ -1353,7 +1372,7 @@ This is a one-time fix-up, please be patient...
// a bit harder to be singletons.
const preferDedupe = this[_preferDedupe] || edge.peer
if (preferDedupe && !tryReplace && dep.canReplace(current)) {
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
const res = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
/* istanbul ignore else - It's extremely rare that a replaceable
* node would be a conflict, if the current one wasn't a conflict,
* but it is theoretically possible if peer deps are pinned. In
Expand Down Expand Up @@ -1421,7 +1440,7 @@ This is a one-time fix-up, please be patient...
}
}
if (canReplace) {
const ret = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath)
const ret = this[_canPlacePeers](dep, target, edge, REPLACE, peerEntryEdge, peerPath, isSource)
/* istanbul ignore else - extremely rare that the peer set would
* conflict if we can replace the node in question, but theoretically
* possible, if peer deps are pinned aggressively. */
Expand Down Expand Up @@ -1482,14 +1501,14 @@ This is a one-time fix-up, please be patient...
}

// no objections! ok to place here
return this[_canPlacePeers](dep, target, edge, OK, peerEntryEdge, peerPath)
return this[_canPlacePeers](dep, target, edge, OK, peerEntryEdge, peerPath, isSource)
}

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

Expand Down
9 changes: 8 additions & 1 deletion lib/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const {normalize} = require('read-package-json-fast')
const {getPaths: getBinPaths} = require('bin-links')
const npa = require('npm-package-arg')
const debug = require('./debug.js')
const gatherDepSet = require('./gather-dep-set.js')

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

// gather up all the deps of this node and that are only depended
// upon by deps of this node. those ones don't count, since
// they'll be replaced if this node is replaced anyway.
const depSet = gatherDepSet([this], e => e.to !== this && e.valid)

for (const edge of this.edgesIn) {
if (!edge.satisfiedBy(node))
// only care about edges that don't originate from this node
if (!depSet.has(edge.from) && !edge.satisfiedBy(node))
return false
}

Expand Down
6 changes: 6 additions & 0 deletions scripts/reify.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ process.on('timeEnd', name => {
}
const res = process.hrtime(timers[name])
delete timers[name]

if (options.quiet)
return

console.error(name, res[0] * 1e3 + res[1] / 1e6)
})
process.on('exit', () => {
Expand Down Expand Up @@ -104,6 +108,8 @@ process.on('log', (level, ...args) => {
if (level === 'warn' && args[0] === 'ERESOLVE') {
args[2] = require('util').inspect(args[2], { depth: Infinity })
}
if (options.quiet)
return
return console.error(level, ...args)
})

Expand Down
Loading

0 comments on commit 3eae280

Please sign in to comment.