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.

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

fix(package-graph): Flatten cycles to avoid skipping packages #2185

Merged
merged 22 commits into from
Jul 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
6faa234
Add wrong test
nicolo-ribaudo Jul 16, 2019
284b9b8
Fix partitionCycles with multiple dependencies
nicolo-ribaudo Jul 17, 2019
a918a22
Add more fixtures
nicolo-ribaudo Jul 17, 2019
d405c84
Update cycles detection logic
nicolo-ribaudo Jul 17, 2019
14ab7a8
Tests
nicolo-ribaudo Jul 17, 2019
031ecf3
[lint] Use fn declarations
nicolo-ribaudo Jul 18, 2019
9c5d045
Revert wrong change in fixture lerna.json
nicolo-ribaudo Jul 18, 2019
8461839
test: cover PackageGraphNode.toString()
evocateur Jul 18, 2019
9072dea
chore: package.json files should be 2-space indented, with trailing s…
evocateur Jul 18, 2019
bf6ee90
Remove externalDependencies map from PackageGraphCollapsedNode, it's …
evocateur Jul 18, 2019
f8c531a
Refactor hasPackageDeep() to return early instead of iterating over _…
evocateur Jul 18, 2019
d06424a
un-nest version-command tests
evocateur Jul 18, 2019
807ea59
test: Always run order-sensitive toposort tests with concurrency of 1…
evocateur Jul 18, 2019
11f9ff4
test: Update expected order of toposort calls
evocateur Jul 18, 2019
0fd8bec
fix arrows in graph models
evocateur Jul 18, 2019
176c0cb
cleanup query-graph
evocateur Jul 18, 2019
eeab59d
Rename PackageGraphCollapsedNode -> CyclicPackageGraphNode
evocateur Jul 18, 2019
35a2b49
cleanup cyclic methods
evocateur Jul 18, 2019
28727a3
Add .unlink() instance method to CyclicPackageGraphNode
evocateur Jul 18, 2019
df7021c
forEach -> for-of
evocateur Jul 18, 2019
794af07
rename methods and parameters for clarity
evocateur Jul 18, 2019
1006a36
s/add/insert/, former method was too Set-like
evocateur Jul 18, 2019
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
17 changes: 17 additions & 0 deletions __fixtures__/cycle-intersection/graph.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
+---+
| A |
+---+
|
v
+---+ +---+ +---+
| E | --> | B | <-- | G |
+---+ +---+ +---+
^ | ^
| v |
+---+ +---+ +---+
| D | <-- | C | --> | F |
+---+ +---+ +---+

Cycles:
B -> C -> D -> E -> B
B -> C -> F -> G -> B
Copy link
Member

Choose a reason for hiding this comment

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

I love the graphs!

3 changes: 3 additions & 0 deletions __fixtures__/cycle-intersection/lerna.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"version": "1.0.0"
}
3 changes: 3 additions & 0 deletions __fixtures__/cycle-intersection/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "cycle-intersection"
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/a/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "a",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/b/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "b",
"version": "1.0.0",
"dependencies": {
"c": "^1.0.0"
}
}
8 changes: 8 additions & 0 deletions __fixtures__/cycle-intersection/packages/c/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "c",
"version": "1.0.0",
"dependencies": {
"d": "^1.0.0",
"f": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/d/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "d",
"version": "1.0.0",
"dependencies": {
"e": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/e/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "e",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/f/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "f",
"version": "1.0.0",
"dependencies": {
"g": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-intersection/packages/g/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "g",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0"
}
}
13 changes: 13 additions & 0 deletions __fixtures__/cycle-parent/graph.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
https://github.com/lerna/lerna/issues/2107#issuecomment-511987691

+---+
/----> | B |
| +---+
|
+---+ +---+ -----\
| A | --> | C | |
+---+ +---+ <-\ |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: C and D's dependency lines should only point one direction each, right?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, I've fixed this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

| | |
| +---+ --/ |
\----> | D | |
+---+ <----/
3 changes: 3 additions & 0 deletions __fixtures__/cycle-parent/lerna.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"version": "1.0.0"
}
3 changes: 3 additions & 0 deletions __fixtures__/cycle-parent/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "cycle-parent"
}
9 changes: 9 additions & 0 deletions __fixtures__/cycle-parent/packages/a/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"name": "a",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0",
"c": "^1.0.0",
"d": "^1.0.0"
}
}
4 changes: 4 additions & 0 deletions __fixtures__/cycle-parent/packages/b/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "b",
"version": "1.0.0"
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-parent/packages/c/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "c",
"version": "1.0.0",
"dependencies": {
"d": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-parent/packages/d/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "d",
"version": "1.0.0",
"dependencies": {
"c": "^1.0.0"
}
}
22 changes: 22 additions & 0 deletions __fixtures__/cycle-separate/graph.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
+---+ +---+
| A | --> | B | <----\
+---+ +---+ |
| | |
v v |
+---+ +---+ +---+
| H | | C | --> | D |
+---+ +---+ +---+
| |
v |
+---+ |
| E | <----\ |
+---+ | |
| | |
v | |
+---+ +---+ |
| F | --> | G | <----/
+---+ +---+

Cycles:
B -> C -> D -> B
E -> F -> G -> E
3 changes: 3 additions & 0 deletions __fixtures__/cycle-separate/lerna.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"version": "1.0.0"
}
3 changes: 3 additions & 0 deletions __fixtures__/cycle-separate/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "cycle-separate"
}
8 changes: 8 additions & 0 deletions __fixtures__/cycle-separate/packages/a/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "a",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0",
"h": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/b/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "b",
"version": "1.0.0",
"dependencies": {
"c": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/c/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "c",
"version": "1.0.0",
"dependencies": {
"d": "^1.0.0"
}
}
8 changes: 8 additions & 0 deletions __fixtures__/cycle-separate/packages/d/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "d",
"version": "1.0.0",
"dependencies": {
"b": "^1.0.0",
"g": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/e/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "e",
"version": "1.0.0",
"dependencies": {
"f": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/f/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "f",
"version": "1.0.0",
"dependencies": {
"g": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/g/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "g",
"version": "1.0.0",
"dependencies": {
"e": "^1.0.0"
}
}
7 changes: 7 additions & 0 deletions __fixtures__/cycle-separate/packages/h/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "h",
"version": "1.0.0",
"dependencies": {
"e": "^1.0.0"
}
}
21 changes: 21 additions & 0 deletions __fixtures__/toposort/graph.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@

+------------------+ +---------+ -----\
| cycle-extraneous | --> | cycle-1 | |
+------------------+ +---------+ <-\ |
| |
+---------+ --/ |
| cycle-2 | |
+---------+ <----/

+--------+
+-------+ --> | dag-2a | ---> +-------+
| dag-3 | +--------+ | dag-1 |
+-------+ ------------------> +-------+
^
+--------+ |
| dag-2b | --/
+--------+

+------------+
| standalone |
+------------+
2 changes: 1 addition & 1 deletion __fixtures__/toposort/package.json
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"name": "independent"
"name": "toposort"
}
8 changes: 2 additions & 6 deletions commands/exec/__tests__/exec-command.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,24 +206,20 @@ describe("ExecCommand", () => {
it("warns when cycles are encountered", async () => {
const testDir = await initFixture("toposort");

await lernaExec(testDir)("ls");
await lernaExec(testDir)("ls", "--concurrency", "1");

const [logMessage] = loggingOutput("warn");
expect(logMessage).toMatch("Dependency cycles detected, you should fix these!");
expect(logMessage).toMatch("package-cycle-1 -> package-cycle-2 -> package-cycle-1");
expect(logMessage).toMatch("package-cycle-2 -> package-cycle-1 -> package-cycle-2");
expect(logMessage).toMatch(
"package-cycle-extraneous -> package-cycle-1 -> package-cycle-2 -> package-cycle-1"
);

expect(calledInPackages()).toEqual([
"package-dag-1",
"package-standalone",
"package-dag-2a",
"package-dag-2b",
"package-dag-3",
"package-cycle-1",
"package-cycle-2",
"package-dag-3",
"package-cycle-extraneous",
]);
});
Expand Down
38 changes: 30 additions & 8 deletions commands/run/__tests__/run-command.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ describe("RunCommand", () => {
it("runs scripts in lexical (not topological) order", async () => {
const testDir = await initFixture("toposort");

await lernaRun(testDir)("env", "--no-sort");
await lernaRun(testDir)("env", "--concurrency", "1", "--no-sort");

expect(output.logged().split("\n")).toEqual([
"package-cycle-1",
Expand All @@ -191,7 +191,7 @@ describe("RunCommand", () => {
it("optionally streams output", async () => {
const testDir = await initFixture("toposort");

await lernaRun(testDir)("env", "--no-sort", "--stream");
await lernaRun(testDir)("env", "--concurrency", "1", "--no-sort", "--stream");

expect(ranInPackagesStreaming(testDir)).toMatchInlineSnapshot(`
Array [
Expand All @@ -212,28 +212,50 @@ describe("RunCommand", () => {
it("warns when cycles are encountered", async () => {
const testDir = await initFixture("toposort");

await lernaRun(testDir)("env");
await lernaRun(testDir)("env", "--concurrency", "1");

const [logMessage] = loggingOutput("warn");
expect(logMessage).toMatch("Dependency cycles detected, you should fix these!");
expect(logMessage).toMatch("package-cycle-1 -> package-cycle-2 -> package-cycle-1");
expect(logMessage).toMatch("package-cycle-2 -> package-cycle-1 -> package-cycle-2");
expect(logMessage).toMatch(
"package-cycle-extraneous -> package-cycle-1 -> package-cycle-2 -> package-cycle-1"
evocateur marked this conversation as resolved.
Show resolved Hide resolved
);

expect(output.logged().split("\n")).toEqual([
"package-dag-1",
"package-standalone",
"package-dag-2a",
"package-dag-2b",
"package-dag-3",
"package-cycle-1",
"package-cycle-2",
"package-dag-3",
evocateur marked this conversation as resolved.
Show resolved Hide resolved
"package-cycle-extraneous",
]);
});

it("works with intersected cycles", async () => {
const testDir = await initFixture("cycle-intersection");

await lernaRun(testDir)("env", "--concurrency", "1");

const [logMessage] = loggingOutput("warn");
expect(logMessage).toMatch("Dependency cycles detected, you should fix these!");
expect(logMessage).toMatch("b -> c -> d -> e -> b");
expect(logMessage).toMatch("f -> g -> (nested cycle: b -> c -> d -> e -> b) -> f");

expect(output.logged().split("\n")).toEqual(["f", "b", "e", "d", "c", "g", "a"]);
});

it("works with separate cycles", async () => {
const testDir = await initFixture("cycle-separate");

await lernaRun(testDir)("env", "--concurrency", "1");

const [logMessage] = loggingOutput("warn");
expect(logMessage).toMatch("Dependency cycles detected, you should fix these!");
expect(logMessage).toMatch("b -> c -> d -> b");
expect(logMessage).toMatch("e -> f -> g -> e");

expect(output.logged().split("\n")).toEqual(["e", "g", "f", "h", "b", "d", "c", "a"]);
});

it("should throw an error with --reject-cycles", async () => {
const testDir = await initFixture("toposort");

Expand Down
29 changes: 29 additions & 0 deletions commands/version/__tests__/version-command.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,35 @@ describe("VersionCommand", () => {
expect(patch).toMatchSnapshot();
});

it("versions all packages with cycles", async () => {
const testDir = await initFixture("cycle-parent");

await gitTag(testDir, "v1.0.0");

await Promise.all(
["a", "b", "c", "d"].map(n => fs.outputFile(path.join(testDir, "packages", n, "index.js"), "hello"))
);
await gitAdd(testDir, ".");
await gitCommit(testDir, "feat: hello");

collectUpdates.mockImplementationOnce(collectUpdatesActual);

await lernaVersion(testDir)("major", "--yes");

const patch = await showCommit(testDir, "--name-only");
expect(patch).toMatchInlineSnapshot(`
"v2.0.0

HEAD -> master, tag: v2.0.0

lerna.json
packages/a/package.json
packages/b/package.json
packages/c/package.json
packages/d/package.json"
`);
});

describe("with relative file: specifiers", () => {
const setupChanges = async (cwd, pkgRoot = "packages") => {
await gitTag(cwd, "v1.0.0");
Expand Down