Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(collect-dependents): Avoid skipping dependents of cyclic dependen…
…cies (#2380)

Replaces the depth-first search with a breadth-first search
  • Loading branch information
fresk-nc authored and evocateur committed Dec 15, 2019
1 parent 0e9bda7 commit bd19a34
Show file tree
Hide file tree
Showing 9 changed files with 53 additions and 17 deletions.
@@ -1,5 +1,5 @@
{
"name": "package-cycle-extraneous",
"name": "package-cycle-extraneous-1",
"version": "1.0.0",
"comment": "This package is used to break ties between package-cycle-{1,2}.",
"dependencies": {
Expand Down
@@ -0,0 +1,9 @@
{
"name": "package-cycle-extraneous-2",
"version": "1.0.0",
"comment": "This package is used to break ties between package-cycle-{1,2}.",
"dependencies": {
"package-cycle-1": "1.0.0",
"package-cycle-2": "1.0.0"
}
}
6 changes: 4 additions & 2 deletions commands/exec/__tests__/exec-command.test.js
Expand Up @@ -200,7 +200,8 @@ describe("ExecCommand", () => {
expect(calledInPackages()).toEqual([
"package-cycle-1",
"package-cycle-2",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
"package-dag-1",
"package-dag-2a",
"package-dag-2b",
Expand Down Expand Up @@ -228,7 +229,8 @@ describe("ExecCommand", () => {
"package-cycle-1",
"package-cycle-2",
"package-dag-3",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
]);
});

Expand Down
9 changes: 6 additions & 3 deletions commands/run/__tests__/run-command.test.js
Expand Up @@ -170,7 +170,8 @@ describe("RunCommand", () => {
expect(output.logged().split("\n")).toEqual([
"package-cycle-1",
"package-cycle-2",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
"package-dag-1",
"package-dag-2a",
"package-dag-2b",
Expand All @@ -188,7 +189,8 @@ describe("RunCommand", () => {
Array [
"packages/package-cycle-1 npm run env (prefixed: true)",
"packages/package-cycle-2 npm run env (prefixed: true)",
"packages/package-cycle-extraneous npm run env (prefixed: true)",
"packages/package-cycle-extraneous-1 npm run env (prefixed: true)",
"packages/package-cycle-extraneous-2 npm run env (prefixed: true)",
"packages/package-dag-1 npm run env (prefixed: true)",
"packages/package-dag-2a npm run env (prefixed: true)",
"packages/package-dag-2b npm run env (prefixed: true)",
Expand Down Expand Up @@ -217,7 +219,8 @@ describe("RunCommand", () => {
"package-cycle-1",
"package-cycle-2",
"package-dag-3",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
]);
});

Expand Down
11 changes: 10 additions & 1 deletion utils/collect-updates/__helpers__/build-graph.js
Expand Up @@ -24,13 +24,22 @@ function buildGraph(mapPackages = pkg => pkg) {
},
},
{
name: "package-cycle-extraneous",
name: "package-cycle-extraneous-1",
version: "1.0.0",
description: "This package is used to break ties between package-cycle-{1,2}.",
dependencies: {
"package-cycle-1": "1.0.0",
},
},
{
name: "package-cycle-extraneous-2",
version: "1.0.0",
description: "This package is used to break ties between package-cycle-{1,2}.",
dependencies: {
"package-cycle-1": "1.0.0",
"package-cycle-2": "1.0.0",
},
},
{
name: "package-dag-1",
version: "1.0.0",
Expand Down
3 changes: 2 additions & 1 deletion utils/collect-updates/__tests__/collect-updates.test.js
Expand Up @@ -40,7 +40,8 @@ makeDiffPredicate.mockImplementation(() => hasDiff);
const ALL_NODES = Object.freeze([
expect.objectContaining({ name: "package-cycle-1" }),
expect.objectContaining({ name: "package-cycle-2" }),
expect.objectContaining({ name: "package-cycle-extraneous" }),
expect.objectContaining({ name: "package-cycle-extraneous-1" }),
expect.objectContaining({ name: "package-cycle-extraneous-2" }),
expect.objectContaining({ name: "package-dag-1" }),
expect.objectContaining({ name: "package-dag-2a" }),
expect.objectContaining({ name: "package-dag-2b" }),
Expand Down
11 changes: 8 additions & 3 deletions utils/collect-updates/__tests__/lib-collect-dependents.test.js
Expand Up @@ -16,8 +16,8 @@ test("source node (dag)", () => {

expect(Array.from(result)).toEqual([
expect.objectContaining({ name: "package-dag-2a" }),
expect.objectContaining({ name: "package-dag-3" }),
expect.objectContaining({ name: "package-dag-2b" }),
expect.objectContaining({ name: "package-dag-3" }),
]);
});

Expand Down Expand Up @@ -53,7 +53,8 @@ test("source node (cycle)", () => {

expect(Array.from(result)).toEqual([
expect.objectContaining({ name: "package-cycle-2" }),
expect.objectContaining({ name: "package-cycle-extraneous" }),
expect.objectContaining({ name: "package-cycle-extraneous-1" }),
expect.objectContaining({ name: "package-cycle-extraneous-2" }),
]);
});

Expand All @@ -65,5 +66,9 @@ test("internal node (cycle)", () => {

const result = collectDependents(candidates);

expect(Array.from(result)).toEqual([expect.objectContaining({ name: "package-cycle-1" })]);
expect(Array.from(result)).toEqual([
expect.objectContaining({ name: "package-cycle-1" }),
expect.objectContaining({ name: "package-cycle-extraneous-2" }),
// package-cycle-extraneous-1 was ignored
]);
});
6 changes: 4 additions & 2 deletions utils/collect-updates/__tests__/lib-collect-packages.test.js
Expand Up @@ -16,7 +16,8 @@ test("returns all packages", () => {
Array [
"package-cycle-1",
"package-cycle-2",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
"package-dag-1",
"package-dag-2a",
"package-dag-2b",
Expand All @@ -38,7 +39,8 @@ test("filters packages through isCandidate, passing node and name", () => {
Array [
"package-cycle-1",
"package-cycle-2",
"package-cycle-extraneous",
"package-cycle-extraneous-1",
"package-cycle-extraneous-2",
]
`);
});
Expand Down
13 changes: 9 additions & 4 deletions utils/collect-updates/lib/collect-dependents.js
Expand Up @@ -11,8 +11,10 @@ function collectDependents(nodes) {
return;
}

// depth-first search
// breadth-first search
const queue = [currentNode];
const seen = new Set();

const visit = (dependentNode, dependentName, siblingDependents) => {
if (seen.has(dependentNode)) {
return;
Expand All @@ -26,11 +28,14 @@ function collectDependents(nodes) {
}

collected.add(dependentNode);

dependentNode.localDependents.forEach(visit);
queue.push(dependentNode);
};

currentNode.localDependents.forEach(visit);
while (queue.length) {
const node = queue.shift();

node.localDependents.forEach(visit);
}
});

return collected;
Expand Down

0 comments on commit bd19a34

Please sign in to comment.