Skip to content

Commit bbcd455

Browse files
fix(arborist): v10 - backport store, lock-only, and override sibling fixes (#9120)
This is a cherry-pick of - 8e0a731 - #9108 - 51365b1 - #9107 - 21ea382 - #9110 and cf34e6f (the changes needed for 21ea382 in v10 branch)
1 parent 49a764e commit bbcd455

File tree

8 files changed

+688
-18
lines changed

8 files changed

+688
-18
lines changed

lib/utils/explain-dep.js

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
const { relative } = require('node:path')
22

3-
const explainNode = (node, depth, chalk) =>
3+
const explainNode = (node, depth, chalk, seen = new Set()) =>
44
printNode(node, chalk) +
5-
explainDependents(node, depth, chalk) +
6-
explainLinksIn(node, depth, chalk)
5+
explainDependents(node, depth, chalk, seen) +
6+
explainLinksIn(node, depth, chalk, seen)
77

88
const colorType = (type, chalk) => {
99
const style = type === 'extraneous' ? chalk.red
@@ -34,24 +34,24 @@ const printNode = (node, chalk) => {
3434
(node.location ? chalk.dim(`\n${node.location}`) : '')
3535
}
3636

37-
const explainLinksIn = ({ linksIn }, depth, chalk) => {
37+
const explainLinksIn = ({ linksIn }, depth, chalk, seen) => {
3838
if (!linksIn || !linksIn.length || depth <= 0) {
3939
return ''
4040
}
4141

42-
const messages = linksIn.map(link => explainNode(link, depth - 1, chalk))
42+
const messages = linksIn.map(link => explainNode(link, depth - 1, chalk, seen))
4343
const str = '\n' + messages.join('\n')
4444
return str.split('\n').join('\n ')
4545
}
4646

47-
const explainDependents = ({ dependents }, depth, chalk) => {
47+
const explainDependents = ({ dependents }, depth, chalk, seen) => {
4848
if (!dependents || !dependents.length || depth <= 0) {
4949
return ''
5050
}
5151

5252
const max = Math.ceil(depth / 2)
5353
const messages = dependents.slice(0, max)
54-
.map(edge => explainEdge(edge, depth, chalk))
54+
.map(edge => explainEdge(edge, depth, chalk, seen))
5555

5656
// show just the names of the first 5 deps that overflowed the list
5757
if (dependents.length > max) {
@@ -75,29 +75,42 @@ const explainDependents = ({ dependents }, depth, chalk) => {
7575
return str.split('\n').join('\n ')
7676
}
7777

78-
const explainEdge = ({ name, type, bundled, from, spec, rawSpec, overridden }, depth, chalk) => {
78+
const explainEdge = (
79+
{ name, type, bundled, from, spec, rawSpec, overridden },
80+
depth, chalk, seen = new Set()
81+
) => {
7982
let dep = type === 'workspace'
8083
? chalk.bold(relative(from.location, spec.slice('file:'.length)))
8184
: `${name}@"${spec}"`
8285
if (overridden) {
8386
dep = `${colorType('overridden', chalk)} ${dep} (was "${rawSpec}")`
8487
}
8588

86-
const fromMsg = ` from ${explainFrom(from, depth, chalk)}`
89+
const fromMsg = ` from ${explainFrom(from, depth, chalk, seen)}`
8790

8891
return (type === 'prod' ? '' : `${colorType(type, chalk)} `) +
8992
(bundled ? `${colorType('bundled', chalk)} ` : '') +
9093
`${dep}${fromMsg}`
9194
}
9295

93-
const explainFrom = (from, depth, chalk) => {
96+
const explainFrom = (from, depth, chalk, seen) => {
9497
if (!from.name && !from.version) {
9598
return 'the root project'
9699
}
97100

98-
return printNode(from, chalk) +
99-
explainDependents(from, depth - 1, chalk) +
100-
explainLinksIn(from, depth - 1, chalk)
101+
// Prevent infinite recursion from cycles in the dependency graph (e.g. linked strategy store nodes). Use stack-based tracking so diamond dependencies (same node reached via different paths) are still explained, but recursive cycles are broken.
102+
const nodeId = `${from.name}@${from.version}:${from.location}`
103+
if (seen.has(nodeId)) {
104+
return printNode(from, chalk)
105+
}
106+
seen.add(nodeId)
107+
108+
const result = printNode(from, chalk) +
109+
explainDependents(from, depth - 1, chalk, seen) +
110+
explainLinksIn(from, depth - 1, chalk, seen)
111+
112+
seen.delete(nodeId)
113+
return result
101114
}
102115

103116
module.exports = { explainNode, printNode, explainEdge }

tap-snapshots/test/lib/utils/explain-dep.js.test.cjs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,28 @@
55
* Make sure to inspect the output below. Do not ignore changes!
66
*/
77
'use strict'
8+
exports[`test/lib/utils/explain-dep.js TAP basic > circular dependency does not recurse infinitely 1`] = `
9+
cycle-a@1.0.0
10+
node_modules/cycle-a
11+
cycle-a@"1.x" from cycle-b@2.0.0
12+
node_modules/cycle-b
13+
cycle-b@"2.x" from cycle-a@1.0.0
14+
node_modules/cycle-a
15+
cycle-a@"1.x" from cycle-b@2.0.0
16+
node_modules/cycle-b
17+
`
18+
19+
exports[`test/lib/utils/explain-dep.js TAP basic > circular dependency from other side 1`] = `
20+
cycle-b@2.0.0
21+
node_modules/cycle-b
22+
cycle-b@"2.x" from cycle-a@1.0.0
23+
node_modules/cycle-a
24+
cycle-a@"1.x" from cycle-b@2.0.0
25+
node_modules/cycle-b
26+
cycle-b@"2.x" from cycle-a@1.0.0
27+
node_modules/cycle-a
28+
`
29+
830
exports[`test/lib/utils/explain-dep.js TAP basic > ellipses test one 1`] = `
931
manydep@1.0.0
1032
manydep@"1.0.0" from prod-dep@1.2.3
@@ -21,6 +43,11 @@ manydep@1.0.0
2143
6 more (optdep, extra-neos, deep-dev, peer, the root project, a package with a pretty long name)
2244
`
2345

46+
exports[`test/lib/utils/explain-dep.js TAP basic > explainEdge without seen parameter 1`] = `
47+
some-dep@"1.x" from parent-pkg@2.0.0
48+
node_modules/parent-pkg
49+
`
50+
2451
exports[`test/lib/utils/explain-dep.js TAP basic bundled > explain color deep 1`] = `
2552
bundle-of-joy@1.0.0 bundled
2653
node_modules/bundle-of-joy

test/lib/utils/explain-dep.js

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
const { resolve } = require('node:path')
22
const t = require('tap')
3-
const { explainNode, printNode } = require('../../../lib/utils/explain-dep.js')
3+
const { explainNode, printNode, explainEdge } = require('../../../lib/utils/explain-dep.js')
44
const { cleanCwd } = require('../../fixtures/clean-snapshot')
55

66
t.cleanSnapshot = (str) => cleanCwd(str)
@@ -268,6 +268,47 @@ t.test('basic', async t => {
268268
})
269269
}
270270

271+
// Regression test for https://github.com/npm/cli/issues/9109
272+
// Circular dependency graphs (common in linked strategy store nodes) should not cause infinite recursion in explainNode.
273+
const cycleA = {
274+
name: 'cycle-a',
275+
version: '1.0.0',
276+
location: 'node_modules/cycle-a',
277+
dependents: [],
278+
}
279+
const cycleB = {
280+
name: 'cycle-b',
281+
version: '2.0.0',
282+
location: 'node_modules/cycle-b',
283+
dependents: [{
284+
type: 'prod',
285+
name: 'cycle-b',
286+
spec: '2.x',
287+
from: cycleA,
288+
}],
289+
}
290+
cycleA.dependents = [{
291+
type: 'prod',
292+
name: 'cycle-a',
293+
spec: '1.x',
294+
from: cycleB,
295+
}]
296+
t.matchSnapshot(explainNode(cycleA, Infinity, noColor), 'circular dependency does not recurse infinitely')
297+
t.matchSnapshot(explainNode(cycleB, Infinity, noColor), 'circular dependency from other side')
298+
299+
// explainEdge called without seen parameter (covers default seen = new Set() branch on explainEdge and explainFrom)
300+
t.matchSnapshot(explainEdge({
301+
type: 'prod',
302+
name: 'some-dep',
303+
spec: '1.x',
304+
from: {
305+
name: 'parent-pkg',
306+
version: '2.0.0',
307+
location: 'node_modules/parent-pkg',
308+
dependents: [],
309+
},
310+
}, 2, noColor), 'explainEdge without seen parameter')
311+
271312
// make sure that we show the last one if it's the only one that would
272313
// hit the ...
273314
cases.manyDeps.dependents.pop()

workspaces/arborist/lib/arborist/reify.js

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,9 +139,11 @@ module.exports = cls => class Reifier extends cls {
139139
// of Node/Link trees
140140
log.warn('reify', 'The "linked" install strategy is EXPERIMENTAL and may contain bugs.')
141141
this.idealTree = await this.createIsolatedTree()
142-
this.#linkedActualForDiff = this.#buildLinkedActualForDiff(
143-
this.idealTree, this.actualTree
144-
)
142+
if (this.actualTree) {
143+
this.#linkedActualForDiff = this.#buildLinkedActualForDiff(
144+
this.idealTree, this.actualTree
145+
)
146+
}
145147
}
146148
await this[_diffTrees]()
147149
await this[_reifyPackages]()
@@ -849,6 +851,10 @@ module.exports = cls => class Reifier extends cls {
849851
if (combined.has(child.path) || !existsSync(child.path)) {
850852
continue
851853
}
854+
// Skip store links whose ideal realpath doesn't exist on disk yet — the store hash changed and the symlink needs recreating via ADD.
855+
if (child.isLink && child.resolved?.startsWith('file:.store/') && !existsSync(child.realpath)) {
856+
continue
857+
}
852858
let entry
853859
if (child.isLink) {
854860
entry = new IsolatedLink(child)

workspaces/arborist/lib/override-set.js

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const npa = require('npm-package-arg')
22
const semver = require('semver')
3+
const { log } = require('proc-log')
34

45
class OverrideSet {
56
constructor ({ overrides, key, parent }) {
@@ -44,6 +45,43 @@ class OverrideSet {
4445
}
4546
}
4647

48+
childrenAreEqual (other) {
49+
if (this.children.size !== other.children.size) {
50+
return false
51+
}
52+
for (const [key] of this.children) {
53+
if (!other.children.has(key)) {
54+
return false
55+
}
56+
if (this.children.get(key).value !== other.children.get(key).value) {
57+
return false
58+
}
59+
if (!this.children.get(key).childrenAreEqual(other.children.get(key))) {
60+
return false
61+
}
62+
}
63+
return true
64+
}
65+
66+
isEqual (other) {
67+
if (this === other) {
68+
return true
69+
}
70+
if (!other) {
71+
return false
72+
}
73+
if (this.key !== other.key || this.value !== other.value) {
74+
return false
75+
}
76+
if (!this.childrenAreEqual(other)) {
77+
return false
78+
}
79+
if (!this.parent) {
80+
return !other.parent
81+
}
82+
return this.parent.isEqual(other.parent)
83+
}
84+
4785
getEdgeRule (edge) {
4886
for (const rule of this.ruleset.values()) {
4987
if (rule.name !== edge.name) {
@@ -142,6 +180,123 @@ class OverrideSet {
142180

143181
return ruleset
144182
}
183+
184+
static findSpecificOverrideSet (first, second) {
185+
for (let overrideSet = second; overrideSet; overrideSet = overrideSet.parent) {
186+
if (overrideSet.isEqual(first)) {
187+
return second
188+
}
189+
}
190+
for (let overrideSet = first; overrideSet; overrideSet = overrideSet.parent) {
191+
if (overrideSet.isEqual(second)) {
192+
return first
193+
}
194+
}
195+
196+
// The override sets are incomparable (e.g. siblings like the "react" and "react-dom" children of the root override set). Check if they have semantically conflicting rules before treating this as an error.
197+
if (this.haveConflictingRules(first, second)) {
198+
log.silly('Conflicting override sets', first, second)
199+
return undefined
200+
}
201+
202+
// The override sets are structurally incomparable but have compatible rules. Fall back to their nearest common ancestor so the node still has a valid override set.
203+
return this.findCommonAncestor(first, second)
204+
}
205+
206+
static findCommonAncestor (first, second) {
207+
const firstAncestors = []
208+
for (const ancestor of first.ancestry()) {
209+
firstAncestors.push(ancestor)
210+
}
211+
for (const secondAnc of second.ancestry()) {
212+
for (const firstAnc of firstAncestors) {
213+
if (firstAnc.isEqual(secondAnc)) {
214+
return firstAnc
215+
}
216+
}
217+
}
218+
return null
219+
}
220+
221+
static doOverrideSetsConflict (first, second) {
222+
// If override sets contain one another then we can try to use the more specific one.
223+
// If neither one is more specific, check for semantic conflicts.
224+
const specificSet = this.findSpecificOverrideSet(first, second)
225+
if (specificSet !== undefined) {
226+
// One contains the other, so no conflict
227+
return false
228+
}
229+
230+
// The override sets are structurally incomparable, but this doesn't necessarily
231+
// mean they conflict. We need to check if they have conflicting version requirements
232+
// for any package that appears in both rulesets.
233+
return this.haveConflictingRules(first, second)
234+
}
235+
236+
static haveConflictingRules (first, second) {
237+
// Get all rules from both override sets
238+
const firstRules = first.ruleset
239+
const secondRules = second.ruleset
240+
241+
// Check each package that appears in both rulesets
242+
for (const [key, firstRule] of firstRules) {
243+
const secondRule = secondRules.get(key)
244+
if (!secondRule) {
245+
// Package only appears in one ruleset, no conflict
246+
continue
247+
}
248+
249+
// Same rule object means no conflict
250+
if (firstRule === secondRule || firstRule.isEqual(secondRule)) {
251+
continue
252+
}
253+
254+
// Both rulesets have rules for this package with different values.
255+
// Check if the version requirements are actually incompatible.
256+
const firstValue = firstRule.value
257+
const secondValue = secondRule.value
258+
259+
// If either value is a reference (starts with $), we can't determine
260+
// compatibility here - the reference might resolve to compatible versions.
261+
// We defer to runtime resolution rather than failing early.
262+
if (firstValue.startsWith('$') || secondValue.startsWith('$')) {
263+
continue
264+
}
265+
266+
// Check if the version ranges are compatible using semver
267+
// If both specify version ranges, they conflict only if they have no overlap
268+
try {
269+
const firstSpec = npa(`${firstRule.name}@${firstValue}`)
270+
const secondSpec = npa(`${secondRule.name}@${secondValue}`)
271+
272+
// For range/version types, check if they intersect
273+
if ((firstSpec.type === 'range' || firstSpec.type === 'version') &&
274+
(secondSpec.type === 'range' || secondSpec.type === 'version')) {
275+
// Check if the ranges intersect
276+
const firstRange = firstSpec.fetchSpec
277+
const secondRange = secondSpec.fetchSpec
278+
279+
// If the ranges don't intersect, we have a real conflict
280+
if (!semver.intersects(firstRange, secondRange)) {
281+
log.silly('Found conflicting override rules', {
282+
package: firstRule.name,
283+
first: firstValue,
284+
second: secondValue,
285+
})
286+
return true
287+
}
288+
}
289+
// For other types (git, file, directory, tag), we can't easily determine
290+
// compatibility, so we conservatively assume no conflict
291+
} catch {
292+
// If we can't parse the specs, conservatively assume no conflict
293+
// Real conflicts will be caught during dependency resolution
294+
}
295+
}
296+
297+
// No conflicting rules found
298+
return false
299+
}
145300
}
146301

147302
module.exports = OverrideSet

0 commit comments

Comments
 (0)