Skip to content

Commit 4b15635

Browse files
darscandarscan
committed
fix: Fix countPathsToRoot for cyclic graphs
`countPathsToRoot` reported incorrect results for cyclic graphs due to the interaction between caching and cycle breaking. You can't easily cache counts for intermediate traversal paths due to contextual cycle breaking, so just cache the final traversal counts.
1 parent cf57280 commit 4b15635

7 files changed

+308
-141
lines changed

‎src/core/dep-graph.ts

+16-24
Original file line numberDiff line numberDiff line change
@@ -152,9 +152,14 @@ class DepGraphImpl implements types.DepGraphInternal {
152152
public countPathsToRoot(pkg: types.Pkg): number {
153153
let count = 0;
154154
for (const nodeId of this.getPkgNodeIds(pkg)) {
155-
count += this.countNodePathsToRoot(nodeId);
155+
if (this._countNodePathsToRootCache.has(nodeId)) {
156+
count += this._countNodePathsToRootCache.get(nodeId)!;
157+
} else {
158+
const c = this.countNodePathsToRoot(nodeId);
159+
this._countNodePathsToRootCache.set(nodeId, c);
160+
count += c;
161+
}
156162
}
157-
158163
return count;
159164
}
160165

@@ -367,30 +372,17 @@ class DepGraphImpl implements types.DepGraphInternal {
367372
return allPaths;
368373
}
369374

370-
private countNodePathsToRoot(
371-
nodeId: string,
372-
ancestors: string[] = [],
373-
): number {
374-
if (ancestors.includes(nodeId)) {
375-
return 0;
376-
}
377-
378-
if (this._countNodePathsToRootCache.has(nodeId)) {
379-
return this._countNodePathsToRootCache.get(nodeId) || 0;
380-
}
381-
382-
const parentNodesIds = this.getNodeParentsNodeIds(nodeId);
383-
if (parentNodesIds.length === 0) {
384-
this._countNodePathsToRootCache.set(nodeId, 1);
375+
private countNodePathsToRoot(nodeId: string, visited: string[] = []): number {
376+
if (nodeId === this._rootNodeId) {
385377
return 1;
386378
}
387-
388-
ancestors = ancestors.concat(nodeId);
389-
const count = parentNodesIds.reduce((acc, parentNodeId) => {
390-
return acc + this.countNodePathsToRoot(parentNodeId, ancestors);
391-
}, 0);
392-
393-
this._countNodePathsToRootCache.set(nodeId, count);
379+
visited = visited.concat(nodeId);
380+
let count = 0;
381+
for (const parentNodeId of this.getNodeParentsNodeIds(nodeId)) {
382+
if (!visited.includes(parentNodeId)) {
383+
count += this.countNodePathsToRoot(parentNodeId, visited);
384+
}
385+
}
394386
return count;
395387
}
396388
}

‎test/core/__snapshots__/cycles.test.ts.snap

-83
This file was deleted.

‎test/core/__snapshots__/pkg-paths-to-root.test.ts.snap

+234
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
// Jest Snapshot v1, https://goo.gl/fbAQLP
2+
3+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: a@1 1`] = `
4+
Array [
5+
Array [
6+
Object {
7+
"name": "a",
8+
"version": "1",
9+
},
10+
],
11+
]
12+
`;
13+
14+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: b@2 1`] = `
15+
Array [
16+
Array [
17+
Object {
18+
"name": "b",
19+
"version": "2",
20+
},
21+
Object {
22+
"name": "a",
23+
"version": "1",
24+
},
25+
],
26+
]
27+
`;
28+
29+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: c@3 1`] = `
30+
Array [
31+
Array [
32+
Object {
33+
"name": "c",
34+
"version": "3",
35+
},
36+
Object {
37+
"name": "a",
38+
"version": "1",
39+
},
40+
],
41+
]
42+
`;
43+
44+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: d@4 1`] = `
45+
Array [
46+
Array [
47+
Object {
48+
"name": "d",
49+
"version": "4",
50+
},
51+
Object {
52+
"name": "a",
53+
"version": "1",
54+
},
55+
],
56+
]
57+
`;
58+
59+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: e@5 1`] = `
60+
Array [
61+
Array [
62+
Object {
63+
"name": "e",
64+
"version": "5",
65+
},
66+
Object {
67+
"name": "b",
68+
"version": "2",
69+
},
70+
Object {
71+
"name": "a",
72+
"version": "1",
73+
},
74+
],
75+
Array [
76+
Object {
77+
"name": "e",
78+
"version": "5",
79+
},
80+
Object {
81+
"name": "c",
82+
"version": "3",
83+
},
84+
Object {
85+
"name": "a",
86+
"version": "1",
87+
},
88+
],
89+
Array [
90+
Object {
91+
"name": "e",
92+
"version": "5",
93+
},
94+
Object {
95+
"name": "g",
96+
"version": "7",
97+
},
98+
Object {
99+
"name": "f",
100+
"version": "6",
101+
},
102+
Object {
103+
"name": "d",
104+
"version": "4",
105+
},
106+
Object {
107+
"name": "a",
108+
"version": "1",
109+
},
110+
],
111+
]
112+
`;
113+
114+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: f@6 1`] = `
115+
Array [
116+
Array [
117+
Object {
118+
"name": "f",
119+
"version": "6",
120+
},
121+
Object {
122+
"name": "d",
123+
"version": "4",
124+
},
125+
Object {
126+
"name": "a",
127+
"version": "1",
128+
},
129+
],
130+
Array [
131+
Object {
132+
"name": "f",
133+
"version": "6",
134+
},
135+
Object {
136+
"name": "e",
137+
"version": "5",
138+
},
139+
Object {
140+
"name": "b",
141+
"version": "2",
142+
},
143+
Object {
144+
"name": "a",
145+
"version": "1",
146+
},
147+
],
148+
Array [
149+
Object {
150+
"name": "f",
151+
"version": "6",
152+
},
153+
Object {
154+
"name": "e",
155+
"version": "5",
156+
},
157+
Object {
158+
"name": "c",
159+
"version": "3",
160+
},
161+
Object {
162+
"name": "a",
163+
"version": "1",
164+
},
165+
],
166+
]
167+
`;
168+
169+
exports[`pkgPathsToRoot cycles returns expected paths for all packages: g@7 1`] = `
170+
Array [
171+
Array [
172+
Object {
173+
"name": "g",
174+
"version": "7",
175+
},
176+
Object {
177+
"name": "f",
178+
"version": "6",
179+
},
180+
Object {
181+
"name": "d",
182+
"version": "4",
183+
},
184+
Object {
185+
"name": "a",
186+
"version": "1",
187+
},
188+
],
189+
Array [
190+
Object {
191+
"name": "g",
192+
"version": "7",
193+
},
194+
Object {
195+
"name": "f",
196+
"version": "6",
197+
},
198+
Object {
199+
"name": "e",
200+
"version": "5",
201+
},
202+
Object {
203+
"name": "b",
204+
"version": "2",
205+
},
206+
Object {
207+
"name": "a",
208+
"version": "1",
209+
},
210+
],
211+
Array [
212+
Object {
213+
"name": "g",
214+
"version": "7",
215+
},
216+
Object {
217+
"name": "f",
218+
"version": "6",
219+
},
220+
Object {
221+
"name": "e",
222+
"version": "5",
223+
},
224+
Object {
225+
"name": "c",
226+
"version": "3",
227+
},
228+
Object {
229+
"name": "a",
230+
"version": "1",
231+
},
232+
],
233+
]
234+
`;

0 commit comments

Comments
 (0)