Skip to content

Commit b1aee62

Browse files
fix: dep flag calculation (#8645)
- reverts pruning added in #8431, which incorrectly prunes deps flagged as `peer` and `optional` - these flags don't mean that this node is an optional peer! - reverts much of #8579, which I think mistakenly changed peer dep calculation logic - rewrites calcDepFlags - adds logic to avoid unsetting `extraneous` when following optional peer edges (how #8431, should have been fixed) - updates my prev fix to avoid looking for missing optional peer deps (`if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) {`) - refactors dep flag unsetting and resetting into Node methods - removes `shake out Link target timing issue` test, which was testing code [removed](2db6c08#diff-6778dbd4bbfddaeb827a8d2aa7248d4c9b329229f69e407d5fd487abe16dd942L333) a while back - avoids omitting flaky`selflink` fixture when writing snapshots Fixes #8535
1 parent 0722535 commit b1aee62

File tree

19 files changed

+911
-1759
lines changed

19 files changed

+911
-1759
lines changed

package-lock.json

Lines changed: 202 additions & 32 deletions
Large diffs are not rendered by default.

tap-snapshots/test/lib/commands/ls.js.test.cjs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@ exports[`test/lib/commands/ls.js TAP ignore missing optional deps --json > ls --
99
Array [
1010
"invalid: optional-wrong@3.2.1 {CWD}/prefix/node_modules/optional-wrong",
1111
"missing: peer-missing@1, required by test-npm-ls-ignore-missing-optional@1.2.3",
12+
"extraneous: peer-optional-ok@1.2.3 {CWD}/prefix/node_modules/peer-optional-ok",
1213
"invalid: peer-optional-wrong@3.2.1 {CWD}/prefix/node_modules/peer-optional-wrong",
14+
"extraneous: peer-optional-wrong@3.2.1 {CWD}/prefix/node_modules/peer-optional-wrong",
1315
"invalid: peer-wrong@3.2.1 {CWD}/prefix/node_modules/peer-wrong",
1416
"missing: prod-missing@1, required by test-npm-ls-ignore-missing-optional@1.2.3",
1517
"invalid: prod-wrong@3.2.1 {CWD}/prefix/node_modules/prod-wrong",
@@ -36,8 +38,8 @@ test-npm-ls-ignore-missing-optional@1.2.3 {CWD}/prefix
3638
+-- UNMET DEPENDENCY peer-missing@1
3739
+-- peer-ok@1.2.3
3840
+-- UNMET OPTIONAL DEPENDENCY peer-optional-missing@1
39-
+-- peer-optional-ok@1.2.3
40-
+-- peer-optional-wrong@3.2.1 invalid: "1" from the root project
41+
+-- peer-optional-ok@1.2.3 extraneous
42+
+-- peer-optional-wrong@3.2.1 invalid: "1" from the root project extraneous
4143
+-- peer-wrong@3.2.1 invalid: "1" from the root project
4244
+-- UNMET DEPENDENCY prod-missing@1
4345
+-- prod-ok@1.2.3

workspaces/arborist/lib/arborist/build-ideal-tree.js

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ module.exports = cls => class IdealTreeBuilder extends cls {
351351
filter: node => node,
352352
visit: node => {
353353
for (const edge of node.edgesOut.values()) {
354-
if (!edge.to || !edge.valid) {
354+
if ((!edge.to && edge.type !== 'peerOptional') || !edge.valid) {
355355
this.#depsQueue.push(node)
356356
break // no need to continue the loop after the first hit
357357
}
@@ -754,6 +754,7 @@ This is a one-time fix-up, please be patient...
754754

755755
// have to re-calc dep flags, because the nodes don't have edges
756756
// until their packages get assigned, so everything looks extraneous
757+
resetDepFlags(this.idealTree)
757758
calcDepFlags(this.idealTree)
758759

759760
// yes, yes, this isn't the "original" version, but now that it's been
@@ -1508,11 +1509,7 @@ This is a one-time fix-up, please be patient...
15081509
} else {
15091510
// otherwise just unset all the flags on the root node
15101511
// since they will sometimes have the default value
1511-
this.idealTree.extraneous = false
1512-
this.idealTree.dev = false
1513-
this.idealTree.optional = false
1514-
this.idealTree.devOptional = false
1515-
this.idealTree.peer = false
1512+
this.idealTree.unsetDepFlags()
15161513
}
15171514

15181515
// at this point, any node marked as extraneous should be pruned.
@@ -1555,12 +1552,7 @@ This is a one-time fix-up, please be patient...
15551552

15561553
#idealTreePrune () {
15571554
for (const node of this.idealTree.inventory.values()) {
1558-
// optional peer dependencies are meant to be added to the tree
1559-
// through an explicit required dependency (most commonly in the
1560-
// root package.json), at which point they won't be optional so
1561-
// any dependencies still marked as both optional and peer at
1562-
// this point can be pruned as a special kind of extraneous
1563-
if (node.extraneous || (node.peer && node.optional)) {
1555+
if (node.extraneous) {
15641556
node.parent = null
15651557
}
15661558
}

workspaces/arborist/lib/arborist/load-virtual.js

Lines changed: 9 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -69,11 +69,7 @@ module.exports = cls => class VirtualLoader extends cls {
6969
if (!this.#rootOptionProvided) {
7070
// root is never any of these things, but might be a brand new
7171
// baby Node object that never had its dep flags calculated.
72-
root.extraneous = false
73-
root.dev = false
74-
root.optional = false
75-
root.devOptional = false
76-
root.peer = false
72+
root.unsetDepFlags()
7773
} else {
7874
this[flagsSuspect] = true
7975
}
@@ -93,11 +89,7 @@ module.exports = cls => class VirtualLoader extends cls {
9389
if (node.isRoot || node === this.#rootOptionProvided) {
9490
continue
9591
}
96-
node.extraneous = true
97-
node.dev = true
98-
node.optional = true
99-
node.devOptional = true
100-
node.peer = true
92+
node.resetDepFlags()
10193
}
10294
calcDepFlags(this.virtualTree, !this.#rootOptionProvided)
10395
}
@@ -255,11 +247,6 @@ To fix:
255247
sw.name = nameFromFolder(path)
256248
}
257249

258-
const dev = sw.dev
259-
const optional = sw.optional
260-
const devOptional = dev || optional || sw.devOptional
261-
const peer = sw.peer
262-
263250
const node = new Node({
264251
installLinks: this.installLinks,
265252
legacyPeerDeps: this.legacyPeerDeps,
@@ -270,18 +257,15 @@ To fix:
270257
resolved: consistentResolve(sw.resolved, this.path, path),
271258
pkg: sw,
272259
hasShrinkwrap: sw.hasShrinkwrap,
273-
dev,
274-
optional,
275-
devOptional,
276-
peer,
277260
loadOverrides,
261+
// cast to boolean because they're undefined in the lock file when false
262+
extraneous: !!sw.extraneous,
263+
devOptional: !!(sw.devOptional || sw.dev || sw.optional),
264+
peer: !!sw.peer,
265+
optional: !!sw.optional,
266+
dev: !!sw.dev,
278267
})
279-
// cast to boolean because they're undefined in the lock file when false
280-
node.extraneous = !!sw.extraneous
281-
node.devOptional = !!(sw.devOptional || sw.dev || sw.optional)
282-
node.peer = !!sw.peer
283-
node.optional = !!sw.optional
284-
node.dev = !!sw.dev
268+
285269
return node
286270
}
287271

workspaces/arborist/lib/calc-dep-flags.js

Lines changed: 86 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -1,144 +1,102 @@
1-
const { depth } = require('treeverse')
2-
1+
// Dep flag (dev, peer, etc.) calculation requires default or reset flags.
2+
// Flags are true by default and are unset to false as we walk deps.
3+
// We iterate outward edges looking for dep flags that can
4+
// be unset based on the current nodes flags and edge type.
5+
// Examples:
6+
// - a non-optional node with a non-optional edge out, the edge node should not be optional
7+
// - a non-peer node with a non-peer edge out, the edge node should not be peer
8+
// If a node is changed, we add to the queue and continue until no more changes.
9+
// Flags that remain after all this unsetting should be valid.
10+
// Examples:
11+
// - a node still flagged optional must only be reachable via optional edges
12+
// - a node still flagged peer must only be reachable via peer edges
313
const calcDepFlags = (tree, resetRoot = true) => {
414
if (resetRoot) {
5-
tree.dev = false
6-
tree.optional = false
7-
tree.devOptional = false
8-
tree.peer = false
15+
tree.unsetDepFlags()
916
}
10-
const ret = depth({
11-
tree,
12-
visit: node => calcDepFlagsStep(node),
13-
filter: node => node,
14-
getChildren: (node, tree) =>
15-
[...tree.edgesOut.values()].map(edge => edge.to),
16-
})
17-
return ret
18-
}
19-
20-
const calcDepFlagsStep = (node) => {
21-
// This rewalk is necessary to handle cases where devDep and optional
22-
// or normal dependency graphs overlap deep in the dep graph.
23-
// Since we're only walking through deps that are not already flagged
24-
// as non-dev/non-optional, it's typically a very shallow traversal
25-
26-
node.extraneous = false
27-
resetParents(node, 'extraneous')
28-
resetParents(node, 'dev')
29-
resetParents(node, 'peer')
30-
resetParents(node, 'devOptional')
31-
resetParents(node, 'optional')
32-
33-
// for links, map their hierarchy appropriately
34-
if (node.isLink) {
35-
// node.target can be null, we check to ensure it's not null before proceeding
36-
if (node.target == null) {
37-
return node
38-
}
39-
node.target.dev = node.dev
40-
node.target.optional = node.optional
41-
node.target.devOptional = node.devOptional
42-
node.target.peer = node.peer
43-
return calcDepFlagsStep(node.target)
44-
}
45-
46-
node.edgesOut.forEach(({ peer, optional, dev, to }) => {
47-
// if the dep is missing, then its flags are already maximally unset
48-
if (!to) {
49-
return
50-
}
51-
// everything with any kind of edge into it is not extraneous
52-
to.extraneous = false
53-
54-
// If this is a peer edge, mark the target as peer
55-
if (peer) {
56-
to.peer = true
57-
} else if (to.peer && !hasIncomingPeerEdge(to)) {
58-
unsetFlag(to, 'peer')
59-
}
6017

61-
// devOptional is the *overlap* of the dev and optional tree.
62-
// however, for convenience and to save an extra rewalk, we leave
63-
// it set when we are in *either* tree, and then omit it from the
64-
// package-lock if either dev or optional are set.
65-
const unsetDevOpt = !node.devOptional && !node.dev && !node.optional && !dev && !optional
18+
const seen = new Set()
19+
const queue = [tree]
6620

67-
// if we are not in the devOpt tree, then we're also not in
68-
// either the dev or opt trees
69-
const unsetDev = unsetDevOpt || !node.dev && !dev
70-
const unsetOpt = unsetDevOpt || !node.optional && !optional
21+
let node
22+
while (node = queue.pop()) {
23+
seen.add(node)
7124

72-
if (unsetDevOpt) {
73-
unsetFlag(to, 'devOptional')
25+
// Unset extraneous from all parents to avoid removal of children.
26+
if (!node.extraneous) {
27+
for (let n = node.resolveParent; n?.extraneous; n = n.resolveParent) {
28+
n.extraneous = false
29+
}
7430
}
7531

76-
if (unsetDev) {
77-
unsetFlag(to, 'dev')
32+
// for links, map their hierarchy appropriately
33+
if (node.isLink) {
34+
// node.target can be null, we check to ensure it's not null before proceeding
35+
if (node.target == null) {
36+
continue
37+
}
38+
node.target.dev = node.dev
39+
node.target.optional = node.optional
40+
node.target.devOptional = node.devOptional
41+
node.target.peer = node.peer
42+
node.target.extraneous = node.extraneous
43+
queue.push(node.target)
44+
continue
7845
}
7946

80-
if (unsetOpt) {
81-
unsetFlag(to, 'optional')
82-
}
83-
})
84-
85-
return node
86-
}
87-
88-
const hasIncomingPeerEdge = (node) => {
89-
const target = node.isLink && node.target ? node.target : node
90-
for (const edge of target.edgesIn) {
91-
if (edge.type === 'peer') {
92-
return true
47+
for (const { peer, optional, dev, to } of node.edgesOut.values()) {
48+
// if the dep is missing, then its flags are already maximally unset
49+
if (!to) {
50+
continue
51+
}
52+
53+
let changed = false
54+
55+
// only optional peer dependencies should stay extraneous
56+
if (to.extraneous && !node.extraneous && !(peer && optional)) {
57+
to.extraneous = false
58+
changed = true
59+
}
60+
61+
if (to.dev && !node.dev && !dev) {
62+
to.dev = false
63+
changed = true
64+
}
65+
66+
if (to.optional && !node.optional && !optional) {
67+
to.optional = false
68+
changed = true
69+
}
70+
71+
// devOptional is the *overlap* of the dev and optional tree.
72+
// A node may be depended on by separate dev and optional nodes.
73+
// It SHOULD NOT be removed when pruning dev OR optional.
74+
// It SHOULD be removed when pruning dev AND optional.
75+
// We only unset here if a node is not dev AND not optional because
76+
// if we did unset, it would prevent any overlap deeper in the tree.
77+
// We correct this later by removing if dev OR optional is set.
78+
if (to.devOptional && !node.devOptional && !node.dev && !node.optional && !dev && !optional) {
79+
to.devOptional = false
80+
changed = true
81+
}
82+
83+
if (to.peer && !node.peer && !peer) {
84+
to.peer = false
85+
changed = true
86+
}
87+
88+
if (changed) {
89+
queue.push(to)
90+
}
9391
}
9492
}
95-
return false
96-
}
9793

98-
const resetParents = (node, flag) => {
99-
if (node[flag]) {
100-
return
101-
}
102-
103-
for (let p = node; p && (p === node || p[flag]); p = p.resolveParent) {
104-
p[flag] = false
105-
}
106-
}
107-
108-
// typically a short walk, since it only traverses deps that have the flag set.
109-
const unsetFlag = (node, flag) => {
110-
if (node[flag]) {
111-
node[flag] = false
112-
depth({
113-
tree: node,
114-
visit: node => {
115-
node.extraneous = node[flag] = false
116-
if (node.isLink && node.target) {
117-
node.target.extraneous = node.target[flag] = false
118-
}
119-
},
120-
getChildren: node => {
121-
const children = []
122-
const targetNode = node.isLink && node.target ? node.target : node
123-
for (const edge of targetNode.edgesOut.values()) {
124-
if (edge.to?.[flag]) {
125-
// For the peer flag, only follow peer edges to unset the flag
126-
// Don't propagate peer flag through prod/dev/optional edges
127-
if (flag === 'peer') {
128-
if (edge.type === 'peer') {
129-
children.push(edge.to)
130-
}
131-
} else {
132-
// For other flags, follow prod edges (and peer edges for non-peer flags)
133-
if (edge.type === 'prod' || edge.type === 'peer') {
134-
children.push(edge.to)
135-
}
136-
}
137-
}
138-
}
139-
return children
140-
},
141-
})
94+
// Remove incorrect devOptional flags now that we have walked all deps.
95+
seen.delete(tree)
96+
for (const node of seen.values()) {
97+
if (node.devOptional && (node.dev || node.optional)) {
98+
node.devOptional = false
99+
}
142100
}
143101
}
144102

workspaces/arborist/lib/node.js

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1613,6 +1613,22 @@ class Node {
16131613
[util.inspect.custom] () {
16141614
return this.toJSON()
16151615
}
1616+
1617+
resetDepFlags () {
1618+
this.extraneous = true
1619+
this.dev = true
1620+
this.optional = true
1621+
this.devOptional = true
1622+
this.peer = true
1623+
}
1624+
1625+
unsetDepFlags () {
1626+
this.extraneous = false
1627+
this.dev = false
1628+
this.optional = false
1629+
this.devOptional = false
1630+
this.peer = false
1631+
}
16161632
}
16171633

16181634
module.exports = Node

workspaces/arborist/lib/reset-dep-flags.js

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,6 @@
66
// we can find the set that is actually extraneous.
77
module.exports = tree => {
88
for (const node of tree.inventory.values()) {
9-
node.extraneous = true
10-
node.dev = true
11-
node.devOptional = true
12-
node.peer = true
13-
node.optional = true
9+
node.resetDepFlags()
1410
}
1511
}

0 commit comments

Comments
 (0)