Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(collect-dependents): Avoid skipping dependents of cyclic dependencies #2380

Merged
merged 1 commit into from Dec 15, 2019
Merged

fix(collect-dependents): Avoid skipping dependents of cyclic dependencies #2380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ges/package-cycle-extraneous/package.json → ...s/package-cycle-extraneous-1/package.json
View file
Open in desktop
@@ -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
9 changes: 9 additions & 0 deletions __fixtures__/toposort/packages/package-cycle-extraneous-2/package.json
View file
Open in desktop
@@ -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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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" }),
evocateur marked this conversation as resolved.
Show resolved Hide resolved
]);
});

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
Copy link

@kenrick95 kenrick95 Oct 1, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please excuse me on commenting on old PR, but what is the reason behind ignoring package-cycle-extraneous-1? I know it is directly because siblingDependents.has(currentNode.name) evaluates to true, but what is the intention behind this? I don't fully understand the comment there ("a direct or transitive cycle, skip it"). I hope someone could explain this. Thanks

]);
});
6 changes: 4 additions & 2 deletions utils/collect-updates/__tests__/lib-collect-packages.test.js
View file
Open in desktop
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
View file
Open in desktop
Expand Up @@ -11,8 +11,10 @@ function collectDependents(nodes) {
return;
}

// depth-first search
// breadth-first search
evocateur marked this conversation as resolved.
Show resolved Hide resolved
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