Update cyclic dependancy error message to show the chain of the cycle (#15)

* update cyclic dependancy error to inlude the cycle

* update tests and cyclic dependancy error output

* Change files

* upadte cyclic dep error message

* chore(test): add test to check overlapping cycles

* chore(test): update overlapping cycles test

* chore(refactor): rename hasCycleDFS func to searchForCycleDFS

change/p-graph-2021-09-23-16-29-58-update-cyclic-dep-err-msg.json

@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "update cyclic dependancy error to inlude the cycle",
+  "packageName": "p-graph",
+  "email": "cheruiyotbryan@gmail.com",
+  "dependentChangeType": "patch",
+  "date": "2021-09-23T13:29:58.510Z"
+}

src/PGraph.ts

@@ -37,11 +37,10 @@ export class PGraph {
       throw new Error("We could not find a node in the graph with no dependencies, this likely means there is a cycle including all nodes");
     }
 
-    const hasCycles = graphHasCycles(this.pGraphDependencyMap);
+    const graph = graphHasCycles(this.pGraphDependencyMap);
 
-    if (hasCycles.hasCycle) {
-      const { nodeId, dependsOn, dependedOnBy } = hasCycles.details;
-      throw new Error(`The dependency graph has a cycle at ${nodeId} which depends on ${dependsOn} and is depended on by ${dependedOnBy}`);
+    if (graph.hasCycle) {
+      throw new Error(`A cycle has been detected including the following nodes:\n${graph.cycle.join("\n")}`);
     }
   }
 

src/__tests__/index.tests.ts

@@ -202,7 +202,39 @@ describe("Public API", () => {
       ["C", "D"],
       ["D", "B"],
     ];
-    const expectedErrorMessage = "The dependency graph has a cycle at B which depends on A,D and is depended on by C";
+    const expectedErrorMessage = `A cycle has been detected including the following nodes:
+B
+C
+D`;
+    expect(() => pGraph(nodeMap, dependencies)).toThrow(expectedErrorMessage);
+  });
+
+  it("throws an exception in the first instance of a cycle that has been detected when there are overlapped cycles", async () => {
+    // This is almost the same as the last test, except the root node is not a part of the cycle
+    const nodeMap: PGraphNodeMap = new Map([
+      ["A", { run: () => Promise.resolve() }],
+      ["B", { run: () => Promise.resolve() }],
+      ["C", { run: () => Promise.resolve() }],
+      ["D", { run: () => Promise.resolve() }],
+      ["E", { run: () => Promise.resolve() }],
+      ["F", { run: () => Promise.resolve() }],
+    ]);
+    // B -> C -> E -> F -> D is the first cycle detected
+    const dependencies: DependencyList = [
+      ["A", "B"],
+      ["B", "C"],
+      ["C", "D"],
+      ["D", "B"],
+      ["C", "E"],
+      ["E", "F"],
+      ["F", "D"],
+    ];
+    const expectedErrorMessage = `A cycle has been detected including the following nodes:
+B
+C
+E
+F
+D`
     expect(() => pGraph(nodeMap, dependencies)).toThrow(expectedErrorMessage);
   });
 

src/graphHasCycles.ts

@@ -2,7 +2,7 @@ import { PGraphNodeWithCyclicDependency, PGraphNodeWithNoCyclicDependency, PGrap
 
 /**
  * Checks for any cycles in the dependency graph, returning `{ hasCycle: false }` if no cycles were detected.
- * Otherwise it returns the details of where the cycle was detected.
+ * Otherwise it returns the chain of nodes where the cycle was detected.
  */
 export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDependencies>): PGraphNodeWithCyclicDependency | PGraphNodeWithNoCyclicDependency {
   /**
@@ -13,7 +13,7 @@ export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDe
    */
   const visitMap = new Map<string, boolean>();
 
-  for (const [nodeId, nodes] of pGraphDependencyMap.entries()) {
+  for (const [nodeId] of pGraphDependencyMap.entries()) {
     /**
      * Test whether this node has already been visited or not.
      */
@@ -21,15 +21,9 @@ export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDe
       /**
        * Test whether the sub-graph of this node has cycles.
        */
-      if (hasCycleDFS(pGraphDependencyMap,  visitMap, nodeId)) {
-        return {
-          hasCycle: true,
-          details: {
-            nodeId,
-            dependsOn: Array.from(nodes.dependsOn),
-            dependedOnBy: Array.from(nodes.dependedOnBy)
-          }
-        };
+      const cycle = searchForCycleDFS(pGraphDependencyMap,  visitMap, nodeId);
+      if (cycle.length) {
+        return { hasCycle: true, cycle };
       }
     }
   }
@@ -54,7 +48,7 @@ interface StackElement {
   traversing: boolean;
 }
 
-const hasCycleDFS = (graph: Map<string, PGraphNodeWithDependencies>, visitMap: Map<string, boolean>, nodeId: string): boolean => {
+const searchForCycleDFS = (graph: Map<string, PGraphNodeWithDependencies>, visitMap: Map<string, boolean>, nodeId: string): string[] => {
   const stack: StackElement[] = [{ node: nodeId, traversing: false }];
   while (stack.length > 0) {
     const current = stack[stack.length - 1];
@@ -65,7 +59,8 @@ const hasCycleDFS = (graph: Map<string, PGraphNodeWithDependencies>, visitMap: M
            * The current node has already been visited,
            * hence there is a cycle.
            */
-          return true;
+          const listOfCycle = stack.filter(i => i.traversing).map(a => a.node);
+          return listOfCycle.slice(listOfCycle.indexOf(current.node));
         } else {
           /**
            * The current node has already been fully traversed
@@ -90,9 +85,9 @@ const hasCycleDFS = (graph: Map<string, PGraphNodeWithDependencies>, visitMap: M
       }
 
       /**
-       * Add the current node's dependencies to the stack
+       * Add the current node's dependents to the stack
        */
-      stack.push(...[...node.dependsOn].map((n) => ({ node: n, traversing: false })));
+      stack.push(...[...node.dependedOnBy].map((n) => ({ node: n, traversing: false })));
     } else {
       /**
        * The current node has now been fully traversed.
@@ -101,5 +96,5 @@ const hasCycleDFS = (graph: Map<string, PGraphNodeWithDependencies>, visitMap: M
       stack.pop();
     }
   }
-  return false;
+  return [];
 };

src/types.ts

@@ -66,21 +66,7 @@ export interface PGraphNodeWithCyclicDependency {
    */
   hasCycle: true;
   /**
-   * Details on where the cyclic dependency was detected.
+   * Chain of node where the cyclic dependency was detected.
    */
-  details: {
-    /**
-     * The identifier of this node, where the cyclic dependency was detected
-     */
-    nodeId: string,
-    /**
-     * The set of nodes that this node depends on.
-     */
-    dependsOn: string[];
-
-    /**
-     * The set of nodes that depend on this node.
-     */
-    dependedOnBy: string[];
-  }
+  cycle: string[]
 }