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

Commit

Permalink
Do not ERESOLVE on peerOptionals not added to tree
Browse files Browse the repository at this point in the history
For these dependency graphs:

    # case a
    root -> (x, y@1)
    x -> PEEROPTIONAL(z)
    z -> PEER(y@2)

    # case b
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEEROPTIONAL(y@2)

    # case c
    root -> (x) PEEROPTIONAL(y@1)
    x -> PEER(z)
    z -> PEEROPTIONAL(y@2)

The peerOptional dependency is included in the peerSet, which would
raise an ERESOLVE conflict at the peerSet generation stage, even though
the peerOptional dependencies will not actually be added to the tree.

To address this, this commit tracks the nodes which are actually
required in the peerSet generation phase, by virtue of being
non-optionally depended upon by a required node in the peerSet.

If a conflict occurs on a node which is not in the required set during
the peerSet generation phase, we ignore it in much the same way that we
would ignore peerSet errors in metadependencies or when --force is used.

Of course, if the peerOptional dependency is _actually_ required, to
avoid a conflict with an invalid resolution present in the tree already,
and there is no suitable placement for it, then ERESOLVE will still be
raised.

Fix: #223
Fix: npm/cli#2667
  • Loading branch information
isaacs committed Feb 11, 2021
1 parent 1bf12bb commit d902354
Show file tree
Hide file tree
Showing 37 changed files with 1,355 additions and 9 deletions.
32 changes: 23 additions & 9 deletions lib/arborist/build-ideal-tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -826,13 +826,18 @@ This is a one-time fix-up, please be patient...
// +-- z@1
// But if x and y are loaded in the same virtual root, then they will
// be forced to agree on a version of z.
const required = edge.type === 'peerOptional' ? new Set()
: new Set([edge.from])
const parent = edge.peer ? virtualRoot : null
const dep = vrDep && vrDep.satisfies(edge) ? vrDep
: await this[_nodeFromEdge](edge, edge.peer ? virtualRoot : null)
: await this[_nodeFromEdge](edge, parent, null, required)

/* istanbul ignore next */
debug(() => {
if (!dep)
throw new Error('no dep??')
})

tasks.push({edge, dep})
}

Expand Down Expand Up @@ -869,7 +874,7 @@ This is a one-time fix-up, please be patient...

// loads a node from an edge, and then loads its peer deps (and their
// peer deps, on down the line) into a virtual root parent.
async [_nodeFromEdge] (edge, parent_, secondEdge = null) {
async [_nodeFromEdge] (edge, parent_, secondEdge, required) {
// create a virtual root node with the same deps as the node that
// is requesting this one, so that we can get all the peer deps in
// a context where they're likely to be resolvable.
Expand Down Expand Up @@ -900,6 +905,11 @@ This is a one-time fix-up, please be patient...
// ensure the one we want is the one that's placed
node.parent = parent

if (required.has(edge.from) && edge.type !== 'peerOptional' ||
secondEdge && (
required.has(secondEdge.from) && secondEdge.type !== 'peerOptional'))
required.add(node)

// handle otherwise unresolvable dependency nesting loops by
// creating a symbolic link
// a1 -> b1 -> a2 -> b2 -> a1 -> ...
Expand All @@ -913,7 +923,7 @@ This is a one-time fix-up, please be patient...
// keep track of the thing that caused this node to be included.
const src = parent.sourceReference
this[_peerSetSource].set(node, src)
return this[_loadPeerSet](node)
return this[_loadPeerSet](node, required)
}

[_virtualRoot] (node, reuse = false) {
Expand Down Expand Up @@ -1058,7 +1068,7 @@ This is a one-time fix-up, please be patient...
// gets placed first. In non-strict mode, we behave strictly if the
// virtual root is based on the root project, and allow non-peer parent
// deps to override, but throw if no preference can be determined.
async [_loadPeerSet] (node) {
async [_loadPeerSet] (node, required) {
const peerEdges = [...node.edgesOut.values()]
// we typically only install non-optional peers, but we have to
// factor them into the peerSet so that we can avoid conflicts
Expand All @@ -1073,10 +1083,12 @@ This is a one-time fix-up, please be patient...
const parentEdge = node.parent.edgesOut.get(edge.name)
const {isProjectRoot, isWorkspace} = node.parent.sourceReference
const isMine = isProjectRoot || isWorkspace
const conflictOK = this[_force] || !isMine && !this[_strictPeerDeps]

if (!edge.to) {
if (!parentEdge) {
// easy, just put the thing there
await this[_nodeFromEdge](edge, node.parent)
await this[_nodeFromEdge](edge, node.parent, null, required)
continue
} else {
// if the parent's edge is very broad like >=1, and the edge in
Expand All @@ -1087,14 +1099,16 @@ This is a one-time fix-up, please be patient...
// a conflict. this is always a problem in strict mode, never
// in force mode, and a problem in non-strict mode if this isn't
// on behalf of our project. in all such cases, we warn at least.
await this[_nodeFromEdge](parentEdge, node.parent, edge)
const dep = await this[_nodeFromEdge](parentEdge, node.parent, edge, required)

// hooray! that worked!
if (edge.valid)
continue

// allow it
if (this[_force] || !isMine && !this[_strictPeerDeps])
// allow it. either we're overriding, or it's not something
// that will be installed by default anyway, and we'll fail when
// we get to the point where we need to, if we need to.
if (conflictOK || !required.has(dep))
continue

// problem
Expand All @@ -1107,7 +1121,7 @@ This is a one-time fix-up, please be patient...
// in non-strict mode if it's not our fault. don't warn here, because
// we are going to warn again when we place the deps, if we end up
// overriding for something else.
if (this[_force] || !isMine && !this[_strictPeerDeps])
if (conflictOK)
continue

// ok, it's the root, or we're in unforced strict mode, so this is bad
Expand Down
Loading

0 comments on commit d902354

Please sign in to comment.