diff --git a/lib/arborist/build-ideal-tree.js b/lib/arborist/build-ideal-tree.js index 7ef42289d..679d52582 100644 --- a/lib/arborist/build-ideal-tree.js +++ b/lib/arborist/build-ideal-tree.js @@ -982,7 +982,6 @@ This is a one-time fix-up, please be patient... // Note that the virtual root will also have virtual copies of the // targets of any child Links, so that they resolve appropriately. const parent = parent_ || this[_virtualRoot](edge.from) - const realParent = edge.peer ? edge.from.resolveParent : edge.from const spec = npa.resolve(edge.name, edge.spec, edge.from.path) const first = await this[_nodeFromSpec](edge.name, spec, parent, edge) @@ -1013,16 +1012,6 @@ This is a one-time fix-up, please be patient... 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 -> ... - // instead of nesting forever, when the loop occurs, create - // a symbolic link to the earlier instance - for (let p = edge.from.resolveParent; p; p = p.resolveParent) { - if (p.matches(node) && !p.isTop) - return new Link({ parent: realParent, target: p }) - } - // keep track of the thing that caused this node to be included. const src = parent.sourceReference this[_peerSetSource].set(node, src) diff --git a/lib/place-dep.js b/lib/place-dep.js index 913b2ba6c..c0023e74a 100644 --- a/lib/place-dep.js +++ b/lib/place-dep.js @@ -16,6 +16,7 @@ const { } = CanPlaceDep const debug = require('./debug.js') +const Link = require('./link.js') const gatherDepSet = require('./gather-dep-set.js') const peerEntrySets = require('./peer-entry-sets.js') @@ -256,6 +257,20 @@ class PlaceDep { return } + // we were told to place it here in the target, so either it does not + // already exist in the tree, OR it's shadowed. + // handle otherwise unresolvable dependency nesting loops by + // creating a symbolic link + // a1 -> b1 -> a2 -> b2 -> a1 -> ... + // instead of nesting forever, when the loop occurs, create + // a symbolic link to the earlier instance + for (let p = target; p; p = p.resolveParent) { + if (p.matches(dep) && !p.isTop) { + this.placed = new Link({ parent: target, target: p }) + return + } + } + // XXX if we are replacing SOME of a peer entry group, we will need to // remove any that are not being replaced and will now be invalid, and // re-evaluate them deeper into the tree. @@ -268,7 +283,7 @@ class PlaceDep { integrity: dep.integrity, legacyPeerDeps: this.legacyPeerDeps, error: dep.errors[0], - ...(dep.isLink ? { target: dep.target, realpath: dep.target.path } : {}), + ...(dep.isLink ? { target: dep.target, realpath: dep.realpath } : {}), }) this.oldDep = target.children.get(this.name) diff --git a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs index 1bb295bc0..3e9558824 100644 --- a/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs +++ b/tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs @@ -76495,7 +76495,7 @@ ArboristNode { } ` -exports[`test/arborist/build-ideal-tree.js TAP pathologically nested dependency cycle > expect resolving Promise 1`] = ` +exports[`test/arborist/build-ideal-tree.js TAP pathologically nested dependency cycle > must match snapshot 1`] = ` ArboristNode { "children": Map { "@isaacs/pathological-dep-nesting-a" => ArboristNode { @@ -76549,27 +76549,6 @@ ArboristNode { "@isaacs/pathological-dep-nesting-b" => ArboristNode { "children": Map { "@isaacs/pathological-dep-nesting-a" => ArboristNode { - "children": Map { - "@isaacs/pathological-dep-nesting-b" => ArboristLink { - "edgesIn": Set { - EdgeIn { - "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a", - "name": "@isaacs/pathological-dep-nesting-b", - "spec": "1", - "type": "prod", - }, - }, - "location": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", - "name": "@isaacs/pathological-dep-nesting-b", - "path": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", - "realpath": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b", - "resolved": "file:../../../../../../../..", - "target": ArboristNode { - "location": "node_modules/@isaacs/pathological-dep-nesting-b", - }, - "version": "1.0.0", - }, - }, "edgesIn": Set { EdgeIn { "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", @@ -76582,7 +76561,7 @@ ArboristNode { "@isaacs/pathological-dep-nesting-b" => EdgeOut { "name": "@isaacs/pathological-dep-nesting-b", "spec": "1", - "to": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a/node_modules/@isaacs/pathological-dep-nesting-b", + "to": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", "type": "prod", }, }, @@ -76592,6 +76571,25 @@ ArboristNode { "resolved": "https://registry.npmjs.org/@isaacs/pathological-dep-nesting-a/-/pathological-dep-nesting-a-1.0.0.tgz", "version": "1.0.0", }, + "@isaacs/pathological-dep-nesting-b" => ArboristLink { + "edgesIn": Set { + EdgeIn { + "from": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-a", + "name": "@isaacs/pathological-dep-nesting-b", + "spec": "1", + "type": "prod", + }, + }, + "location": "node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", + "name": "@isaacs/pathological-dep-nesting-b", + "path": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b/node_modules/@isaacs/pathological-dep-nesting-b", + "realpath": "{CWD}/test/fixtures/pathological-dep-nesting-cycle/node_modules/@isaacs/pathological-dep-nesting-b", + "resolved": "file:../../../../..", + "target": ArboristNode { + "location": "node_modules/@isaacs/pathological-dep-nesting-b", + }, + "version": "1.0.0", + }, }, "edgesIn": Set { EdgeIn { diff --git a/tap-snapshots/test/place-dep.js.test.cjs b/tap-snapshots/test/place-dep.js.test.cjs index 4e6053fdb..b819d4dc3 100644 --- a/tap-snapshots/test/place-dep.js.test.cjs +++ b/tap-snapshots/test/place-dep.js.test.cjs @@ -4369,6 +4369,96 @@ exports[`test/place-dep.js TAP placement tests nest peer set under dependent nod Array [] ` +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > changes to tree 1`] = ` +--- expected ++++ actual +@@ -118,13 +118,6 @@ + "spec": "2", + "from": "node_modules/b/node_modules/a", + }, +- EdgeIn { +- "type": "prod", +- "name": "b", +- "spec": "1", +- "error": "INVALID", +- "from": "node_modules/b/node_modules/b/node_modules/a", +- }, + }, + "children": Map { + "a" => ArboristNode { +@@ -141,8 +134,7 @@ + "type": "prod", + "name": "b", + "spec": "1", +- "to": "node_modules/b/node_modules/b", +- "error": "INVALID", ++ "to": "node_modules/b/node_modules/b/node_modules/b", + }, + }, + "edgesIn": Set { +@@ -154,6 +146,29 @@ + }, + }, + }, ++ "b" => ArboristLink { ++ "name": "b", ++ "version": "1.0.0", ++ "location": "node_modules/b/node_modules/b/node_modules/b", ++ "path": "/some/path/node_modules/b/node_modules/b/node_modules/b", ++ "realpath": "/some/path/node_modules/b", ++ "resolved": "file:../../..", ++ "extraneous": true, ++ "dev": true, ++ "optional": true, ++ "peer": true, ++ "edgesIn": Set { ++ EdgeIn { ++ "type": "prod", ++ "name": "b", ++ "spec": "1", ++ "from": "node_modules/b/node_modules/b/node_modules/a", ++ }, ++ }, ++ "target": ArboristNode { ++ "location": "node_modules/b", ++ }, ++ }, + }, + }, + }, + +` + +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > placements 1`] = ` +Array [ + Object { + "canPlace": Symbol(OK), + "canPlaceSelf": Symbol(OK), + "checks": Map { + "node_modules/b/node_modules/b/node_modules/a" => Array [ + Symbol(OK), + Symbol(OK), + ], + "node_modules/b/node_modules/b" => Array [ + Symbol(OK), + Symbol(OK), + ], + "node_modules/b" => Array [ + Symbol(CONFLICT), + Symbol(CONFLICT), + ], + }, + "dep": "b@1.0.0", + "edge": "{ node_modules/b/node_modules/b/node_modules/a prod b@1 }", + "placed": "node_modules/b/node_modules/b/node_modules/b", + }, +] +` + +exports[`test/place-dep.js TAP placement tests pathologically nested dependency cycle > warnings 1`] = ` +Array [] +` + exports[`test/place-dep.js TAP placement tests peer all the way down, conflict but not ours > changes to tree 1`] = ` --- expected +++ actual diff --git a/test/arborist/build-ideal-tree.js b/test/arborist/build-ideal-tree.js index cec5fc06f..d19366b1d 100644 --- a/test/arborist/build-ideal-tree.js +++ b/test/arborist/build-ideal-tree.js @@ -818,9 +818,10 @@ t.test('update global space single dep', t => { }) // if we get this wrong, it'll spin forever and use up all the memory -t.test('pathologically nested dependency cycle', t => - t.resolveMatchSnapshot(printIdeal( - resolve(fixtures, 'pathological-dep-nesting-cycle')))) +t.test('pathologically nested dependency cycle', async t => { + t.matchSnapshot(await printIdeal( + resolve(fixtures, 'pathological-dep-nesting-cycle'))) +}) t.test('resolve file deps from cwd', t => { const cwd = process.cwd() diff --git a/test/place-dep.js b/test/place-dep.js index 0ee4b279d..fc799b807 100644 --- a/test/place-dep.js +++ b/test/place-dep.js @@ -605,6 +605,34 @@ t.test('placement tests', t => { }, }) + // pathologically nested dep cycle + // a1 -> b1 -> a2 -> b2 -> a1 + runTest('pathologically nested dependency cycle', { + tree: new Node({ + path, + pkg: { dependencies: { a: '1' }}, + children: [ + { pkg: { name: 'a', version: '1.0.0', dependencies: { b: '1' }}}, + { + pkg: { name: 'b', version: '1.0.0', dependencies: { a: '2' }}, + children: [ + { pkg: { name: 'a', version: '2.0.0', dependencies: { b: '2' }}}, + { + pkg: { name: 'b', version: '2.0.0', dependencies: { a: '1' }}, + children: [ + { pkg: { name: 'a', version: '1.0.0', dependencies: { b: '1' }}}, + ], + }, + ], + }, + ], + }), + dep: new Node({ + pkg: { name: 'b', version: '1.0.0', dependencies: { a: '2' }}, + }), + nodeLoc: 'node_modules/b/node_modules/b/node_modules/a', + }) + // peer dep shenanigans runTest('basic placement of a production dep with peer deps', { tree: new Node({