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

fix: move pathological nest fixing link to PlaceDep #309

Closed
wants to merge 1 commit into from
Closed

fix: move pathological nest fixing link to PlaceDep #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 0 additions & 11 deletions lib/arborist/build-ideal-tree.js
View file
Open in desktop
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
17 changes: 16 additions & 1 deletion lib/place-dep.js
View file
Open in desktop
Expand Up @@ -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')

Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand Down
44 changes: 21 additions & 23 deletions tap-snapshots/test/arborist/build-ideal-tree.js.test.cjs
View file
Open in desktop
Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand All @@ -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",
},
},
Expand All @@ -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 {
Expand Down
90 changes: 90 additions & 0 deletions tap-snapshots/test/place-dep.js.test.cjs
View file
Open in desktop
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions test/arborist/build-ideal-tree.js
View file
Open in desktop
Expand Up @@ -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()
Expand Down
28 changes: 28 additions & 0 deletions test/place-dep.js
View file
Open in desktop
Expand Up @@ -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({
Expand Down