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

nicolo-ribaudo · evocateur · commit b335763b38ea · 2019-07-18T12:07:24.000-07:00
diff --git a/__fixtures__/cycle-intersection/graph.txt b/__fixtures__/cycle-intersection/graph.txt
@@ -0,0 +1,17 @@
+          +---+
+          | A |
+          +---+
+            |
+            v
++---+     +---+     +---+
+| E | --> | B | <-- | G |
++---+     +---+     +---+
+  ^         |         ^  
+  |         v         |  
++---+     +---+     +---+
+| D | <-- | C | --> | F |
++---+     +---+     +---+
+
+Cycles:
+B -> C -> D -> E -> B
+B -> C -> F -> G -> B
diff --git a/__fixtures__/cycle-intersection/lerna.json b/__fixtures__/cycle-intersection/lerna.json
@@ -0,0 +1,3 @@
+{
+  "version": "1.0.0"
+}
diff --git a/__fixtures__/cycle-intersection/package.json b/__fixtures__/cycle-intersection/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "cycle-intersection"
+}
diff --git a/__fixtures__/cycle-intersection/packages/a/package.json b/__fixtures__/cycle-intersection/packages/a/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "a",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/b/package.json b/__fixtures__/cycle-intersection/packages/b/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "b",
+  "version": "1.0.0",
+  "dependencies": {
+    "c": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/c/package.json b/__fixtures__/cycle-intersection/packages/c/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "c",
+  "version": "1.0.0",
+  "dependencies": {
+    "d": "^1.0.0",
+    "f": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/d/package.json b/__fixtures__/cycle-intersection/packages/d/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "d",
+  "version": "1.0.0",
+  "dependencies": {
+    "e": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/e/package.json b/__fixtures__/cycle-intersection/packages/e/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "e",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/f/package.json b/__fixtures__/cycle-intersection/packages/f/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "f",
+  "version": "1.0.0",
+  "dependencies": {
+    "g": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-intersection/packages/g/package.json b/__fixtures__/cycle-intersection/packages/g/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "g",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-parent/graph.txt b/__fixtures__/cycle-parent/graph.txt
@@ -0,0 +1,13 @@
+https://github.com/lerna/lerna/issues/2107#issuecomment-511987691
+
+          +---+
+   /----> | B |
+  |       +---+
+  |
++---+     +---+ -----\
+| A | --> | C |       |
++---+     +---+ <-\   |
+  |                |  |
+  |       +---+ --/   |
+   \----> | D |       |
+          +---+ <----/
diff --git a/__fixtures__/cycle-parent/lerna.json b/__fixtures__/cycle-parent/lerna.json
@@ -0,0 +1,3 @@
+{
+  "version": "1.0.0"
+}
diff --git a/__fixtures__/cycle-parent/package.json b/__fixtures__/cycle-parent/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "cycle-parent"
+}
diff --git a/__fixtures__/cycle-parent/packages/a/package.json b/__fixtures__/cycle-parent/packages/a/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "a",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0",
+    "c": "^1.0.0",
+    "d": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-parent/packages/b/package.json b/__fixtures__/cycle-parent/packages/b/package.json
@@ -0,0 +1,4 @@
+{
+  "name": "b",
+  "version": "1.0.0"
+}
diff --git a/__fixtures__/cycle-parent/packages/c/package.json b/__fixtures__/cycle-parent/packages/c/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "c",
+  "version": "1.0.0",
+  "dependencies": {
+    "d": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-parent/packages/d/package.json b/__fixtures__/cycle-parent/packages/d/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "d",
+  "version": "1.0.0",
+  "dependencies": {
+    "c": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/graph.txt b/__fixtures__/cycle-separate/graph.txt
@@ -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
diff --git a/__fixtures__/cycle-separate/lerna.json b/__fixtures__/cycle-separate/lerna.json
@@ -0,0 +1,3 @@
+{
+  "version": "1.0.0"
+}
diff --git a/__fixtures__/cycle-separate/package.json b/__fixtures__/cycle-separate/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "cycle-separate"
+}
diff --git a/__fixtures__/cycle-separate/packages/a/package.json b/__fixtures__/cycle-separate/packages/a/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "a",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0",
+    "h": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/b/package.json b/__fixtures__/cycle-separate/packages/b/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "b",
+  "version": "1.0.0",
+  "dependencies": {
+    "c": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/c/package.json b/__fixtures__/cycle-separate/packages/c/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "c",
+  "version": "1.0.0",
+  "dependencies": {
+    "d": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/d/package.json b/__fixtures__/cycle-separate/packages/d/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "d",
+  "version": "1.0.0",
+  "dependencies": {
+    "b": "^1.0.0",
+    "g": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/e/package.json b/__fixtures__/cycle-separate/packages/e/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "e",
+  "version": "1.0.0",
+  "dependencies": {
+    "f": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/f/package.json b/__fixtures__/cycle-separate/packages/f/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "f",
+  "version": "1.0.0",
+  "dependencies": {
+    "g": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/g/package.json b/__fixtures__/cycle-separate/packages/g/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "g",
+  "version": "1.0.0",
+  "dependencies": {
+    "e": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/cycle-separate/packages/h/package.json b/__fixtures__/cycle-separate/packages/h/package.json
@@ -0,0 +1,7 @@
+{
+  "name": "h",
+  "version": "1.0.0",
+  "dependencies": {
+    "e": "^1.0.0"
+  }
+}
diff --git a/__fixtures__/toposort/graph.txt b/__fixtures__/toposort/graph.txt
@@ -0,0 +1,21 @@
+
++------------------+     +---------+ -----\
+| cycle-extraneous | --> | cycle-1 |       |
++------------------+     +---------+ <-\   |
+                                        |  |
+                         +---------+ --/   |
+                         | cycle-2 |       |
+                         +---------+ <----/
+
+              +--------+
++-------+ --> | dag-2a | ---> +-------+
+| dag-3 |     +--------+      | dag-1 |
++-------+ ------------------> +-------+
+                                 ^
+                   +--------+    |
+                   | dag-2b | --/
+                   +--------+
+
++------------+
+| standalone |
++------------+
diff --git a/__fixtures__/toposort/package.json b/__fixtures__/toposort/package.json
@@ -1,3 +1,3 @@
 {
-  "name": "independent"
+  "name": "toposort"
 }
diff --git a/commands/exec/__tests__/exec-command.test.js b/commands/exec/__tests__/exec-command.test.js
@@ -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",
       ]);
     });
diff --git a/commands/run/__tests__/run-command.test.js b/commands/run/__tests__/run-command.test.js
@@ -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",
@@ -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 [
@@ -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"
-      );
 
       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",
         "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");
 
diff --git a/commands/version/__tests__/version-command.test.js b/commands/version/__tests__/version-command.test.js
@@ -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");
diff --git a/core/package-graph/__tests__/package-graph.test.js b/core/package-graph/__tests__/package-graph.test.js
diff --git a/core/package-graph/index.js b/core/package-graph/index.js
diff --git a/utils/listable/__tests__/listable-format.test.js b/utils/listable/__tests__/listable-format.test.js
diff --git a/utils/query-graph/query-graph.js b/utils/query-graph/query-graph.js

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+{`
	`2`	`+ "name": "cycle-intersection"`
	`3`	`+}`