From bd19a34debf3344e94386b4ffd4b3fe87efb4641 Mon Sep 17 00:00:00 2001 From: George Besedin Date: Mon, 16 Dec 2019 00:46:10 +0300 Subject: [PATCH] fix(collect-dependents): Avoid skipping dependents of cyclic dependencies (#2380) Replaces the depth-first search with a breadth-first search --- .../package.json | 2 +- .../package-cycle-extraneous-2/package.json | 9 +++++++++ commands/exec/__tests__/exec-command.test.js | 6 ++++-- commands/run/__tests__/run-command.test.js | 9 ++++++--- utils/collect-updates/__helpers__/build-graph.js | 11 ++++++++++- .../__tests__/collect-updates.test.js | 3 ++- .../__tests__/lib-collect-dependents.test.js | 11 ++++++++--- .../__tests__/lib-collect-packages.test.js | 6 ++++-- utils/collect-updates/lib/collect-dependents.js | 13 +++++++++---- 9 files changed, 53 insertions(+), 17 deletions(-) rename __fixtures__/toposort/packages/{package-cycle-extraneous => package-cycle-extraneous-1}/package.json (80%) create mode 100644 __fixtures__/toposort/packages/package-cycle-extraneous-2/package.json diff --git a/__fixtures__/toposort/packages/package-cycle-extraneous/package.json b/__fixtures__/toposort/packages/package-cycle-extraneous-1/package.json similarity index 80% rename from __fixtures__/toposort/packages/package-cycle-extraneous/package.json rename to __fixtures__/toposort/packages/package-cycle-extraneous-1/package.json index f7c3918c59..b718b69c79 100644 --- a/__fixtures__/toposort/packages/package-cycle-extraneous/package.json +++ b/__fixtures__/toposort/packages/package-cycle-extraneous-1/package.json @@ -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": { diff --git a/__fixtures__/toposort/packages/package-cycle-extraneous-2/package.json b/__fixtures__/toposort/packages/package-cycle-extraneous-2/package.json new file mode 100644 index 0000000000..e98c791f82 --- /dev/null +++ b/__fixtures__/toposort/packages/package-cycle-extraneous-2/package.json @@ -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" + } +} diff --git a/commands/exec/__tests__/exec-command.test.js b/commands/exec/__tests__/exec-command.test.js index 2ad3662eeb..a82d9ce075 100644 --- a/commands/exec/__tests__/exec-command.test.js +++ b/commands/exec/__tests__/exec-command.test.js @@ -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", @@ -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", ]); }); diff --git a/commands/run/__tests__/run-command.test.js b/commands/run/__tests__/run-command.test.js index a8931c282d..b5f2de723f 100644 --- a/commands/run/__tests__/run-command.test.js +++ b/commands/run/__tests__/run-command.test.js @@ -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", @@ -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)", @@ -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", ]); }); diff --git a/utils/collect-updates/__helpers__/build-graph.js b/utils/collect-updates/__helpers__/build-graph.js index db75e806c8..e426b2c303 100644 --- a/utils/collect-updates/__helpers__/build-graph.js +++ b/utils/collect-updates/__helpers__/build-graph.js @@ -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", diff --git a/utils/collect-updates/__tests__/collect-updates.test.js b/utils/collect-updates/__tests__/collect-updates.test.js index d2af1e1c51..90bf2d4a97 100644 --- a/utils/collect-updates/__tests__/collect-updates.test.js +++ b/utils/collect-updates/__tests__/collect-updates.test.js @@ -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" }), diff --git a/utils/collect-updates/__tests__/lib-collect-dependents.test.js b/utils/collect-updates/__tests__/lib-collect-dependents.test.js index 4594ce3401..4a7a68d30c 100644 --- a/utils/collect-updates/__tests__/lib-collect-dependents.test.js +++ b/utils/collect-updates/__tests__/lib-collect-dependents.test.js @@ -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" }), ]); }); @@ -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" }), ]); }); @@ -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 + ]); }); diff --git a/utils/collect-updates/__tests__/lib-collect-packages.test.js b/utils/collect-updates/__tests__/lib-collect-packages.test.js index 9fa6980647..6e366eab21 100644 --- a/utils/collect-updates/__tests__/lib-collect-packages.test.js +++ b/utils/collect-updates/__tests__/lib-collect-packages.test.js @@ -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", @@ -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", ] `); }); diff --git a/utils/collect-updates/lib/collect-dependents.js b/utils/collect-updates/lib/collect-dependents.js index b94dc0718b..799a7e7579 100644 --- a/utils/collect-updates/lib/collect-dependents.js +++ b/utils/collect-updates/lib/collect-dependents.js @@ -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; @@ -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;