Add descriptive error message when there is a cyclic dependancy (#9)

* add descriptive error message when there is a cyclic dependancy

* Change files

.gitignore

@@ -1,3 +1,4 @@
 node_modules
 lib
-*.log
+*.log
+.idea

change/p-graph-2021-04-09-15-05-29-brycecee-detailed-error-message.json

@@ -0,0 +1,8 @@
+{
+  "type": "patch",
+  "comment": "add descriptive error message when there is a cyclic dependancy",
+  "packageName": "p-graph",
+  "email": "cheruiyotbryan@gmail.com",
+  "dependentChangeType": "patch",
+  "date": "2021-04-09T12:05:29.808Z"
+}

src/PGraph.ts

@@ -37,8 +37,11 @@ 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");
     }
 
-    if (graphHasCycles(this.pGraphDependencyMap)) {
-      throw new Error("The dependency graph has a cycle in it");
+    const hasCycles = 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}`);
     }
   }
 
@@ -81,7 +84,7 @@ export class PGraph {
       } finally {
         // schedule next round of tasks if options.continue (continue on error) or successfully run task
         const shouldScheduleMoreTasks = options?.continue || !taskToRun.failed;
-     
+
         if (shouldScheduleMoreTasks) {
           // "currentlyRunningTaskCount" cannot be decremented on non-continue cases because of async nature of
           // the queue runner. The race condition will end up appearing as if there was no failures even though
@@ -102,7 +105,7 @@ export class PGraph {
             if (dependentNode.dependsOn.size === 0) {
               priorityQueue.insert(dependentId, nodeCumulativePriorities.get(dependentId)!);
             }
-          });        
+          });
         }
       }
     };
@@ -120,7 +123,7 @@ export class PGraph {
           }
           return;
         }
-        
+
         while (!priorityQueue.isEmpty() && (concurrency === undefined || currentlyRunningTaskCount < concurrency)) {
           scheduleTask()
             .then(() => trySchedulingTasks())
@@ -128,7 +131,7 @@ export class PGraph {
               errors.push(e);
 
               // if a continue option is set, this merely records what errors have been encountered
-              // it'll continue down the execution until all the tasks that still works 
+              // it'll continue down the execution until all the tasks that still works
               if (options?.continue) {
                 trySchedulingTasks();
               } else {

src/__tests__/index.tests.ts

@@ -187,7 +187,7 @@ describe("Public API", () => {
     expect(() => pGraph(nodeMap, dependencies)).toThrow();
   });
 
-  it("throws an exception when the dependency graph has a cycle", async () => {
+  it("throws an exception with detailed message when the dependency graph has a cycle", 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() }],
@@ -202,8 +202,8 @@ describe("Public API", () => {
       ["C", "D"],
       ["D", "B"],
     ];
-
-    expect(() => pGraph(nodeMap, dependencies)).toThrow();
+    const expectedErrorMessage = "The dependency graph has a cycle at B which depends on A,D and is depended on by C";
+    expect(() => pGraph(nodeMap, dependencies)).toThrow(expectedErrorMessage);
   });
 
   it("resolves an empty dependnecy graph", async () => {

src/graphHasCycles.ts

@@ -1,9 +1,10 @@
-import { PGraphNodeWithDependencies } from "./types";
+import { PGraphNodeWithCyclicDependency, PGraphNodeWithNoCyclicDependency, PGraphNodeWithDependencies } from "./types";
 
 /**
- * Checks for any cycles in the dependency graph, returning false if no cycles were detected.
+ * 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.
  */
-export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDependencies>): boolean {
+export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDependencies>): PGraphNodeWithCyclicDependency | PGraphNodeWithNoCyclicDependency {
   /**
    *  A map to keep track of the visited and visiting nodes.
    * <node, true> entry means it is currently being visited.
@@ -12,7 +13,7 @@ export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDe
    */
   const visitMap = new Map<string, boolean>();
 
-  for (const [nodeId] of pGraphDependencyMap.entries()) {
+  for (const [nodeId, nodes] of pGraphDependencyMap.entries()) {
     /**
      * Test whether this node has already been visited or not.
      */
@@ -20,13 +21,20 @@ export function graphHasCycles(pGraphDependencyMap: Map<string, PGraphNodeWithDe
       /**
        * Test whether the sub-graph of this node has cycles.
        */
-      if (hasCycleDFS(pGraphDependencyMap, visitMap, nodeId)) {
-        return true;
+      if (hasCycleDFS(pGraphDependencyMap,  visitMap, nodeId)) {
+        return {
+          hasCycle: true,
+          details: {
+            nodeId,
+            dependsOn: Array.from(nodes.dependsOn),
+            dependedOnBy: Array.from(nodes.dependedOnBy)
+          }
+        };
       }
     }
   }
 
-  return false;
+  return { hasCycle: false };
 }
 
 /**

src/types.ts

@@ -52,3 +52,35 @@ export interface PGraphNodeWithDependencies extends PGraphNode {
    */
   failed: boolean;
 }
+
+export interface PGraphNodeWithNoCyclicDependency {
+  /**
+   * Flag whether there is no cyclic dependency
+   */
+  hasCycle: false;
+}
+
+export interface PGraphNodeWithCyclicDependency {
+  /**
+   * Flag whether there is a cyclic dependency
+   */
+  hasCycle: true;
+  /**
+   * Details on 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[];
+  }
+}