fix: make dependency graph more robust (#909)

Fixes #898

Needed for easier debugging (script to clone a jsr package and all its
dependencies locally)

Author: crowlKats

api/src/api/package.rs

@@ -26,6 +26,7 @@ use hyper::Request;
 use hyper::Response;
 use hyper::StatusCode;
 use indexmap::IndexMap;
+use indexmap::IndexSet;
 use regex::Regex;
 use routerify::prelude::RequestExt;
 use routerify::Router;
@@ -1849,6 +1850,7 @@ struct GraphDependencyCollector<'a> {
   dependencies: &'a mut IndexMap<DependencyKind, DependencyInfo>,
   exports: &'a IndexMap<String, IndexMap<String, String>>,
   id_index: &'a mut usize,
+  visited: IndexSet<DependencyKind>,
 }
 
 impl<'a> GraphDependencyCollector<'a> {
@@ -1866,6 +1868,7 @@ impl<'a> GraphDependencyCollector<'a> {
       dependencies,
       exports,
       id_index,
+      visited: Default::default(),
     }
     .build_module_info(root_module)
     .unwrap();
@@ -1920,6 +1923,12 @@ impl<'a> GraphDependencyCollector<'a> {
       }
     };
 
+    if self.visited.contains(&dependency) {
+      return self.dependencies.get(&dependency).map(|dep| dep.id);
+    } else {
+      self.visited.insert(dependency.clone());
+    }
+
     if let Some(info) = self.dependencies.get(&dependency) {
       Some(info.id)
     } else {
@@ -1941,24 +1950,27 @@ impl<'a> GraphDependencyCollector<'a> {
         | Module::External(_) => None,
       };
 
-      let mut children = vec![];
+      let id = *self.id_index;
+      *self.id_index += 1;
+
+      let mut children = IndexSet::new();
       match module {
         Module::Js(module) => {
           if let Some(types_dep) = &module.maybe_types_dependency {
             if let Some(child) = self.build_resolved_info(&types_dep.dependency)
             {
-              children.push(child);
+              children.insert(child);
             }
           }
           for dep in module.dependencies.values() {
             if !dep.maybe_code.is_none() {
               if let Some(child) = self.build_resolved_info(&dep.maybe_code) {
-                children.push(child);
+                children.insert(child);
               }
             }
             if !dep.maybe_type.is_none() {
               if let Some(child) = self.build_resolved_info(&dep.maybe_type) {
-                children.push(child);
+                children.insert(child);
               }
             }
           }
@@ -1970,8 +1982,6 @@ impl<'a> GraphDependencyCollector<'a> {
         | Module::External(_) => {}
       }
 
-      let id = *self.id_index;
-
       self.dependencies.insert(
         dependency,
         DependencyInfo {
@@ -1982,8 +1992,6 @@ impl<'a> GraphDependencyCollector<'a> {
         },
       );
 
-      *self.id_index += 1;
-
       Some(id)
     }
   }
@@ -2004,7 +2012,7 @@ impl<'a> GraphDependencyCollector<'a> {
               },
               DependencyInfo {
                 id,
-                children: vec![],
+                children: Default::default(),
                 size: None,
                 media_type: None,
               },
@@ -2052,7 +2060,7 @@ pub enum DependencyKind {
 #[derive(Debug, Eq, PartialEq)]
 pub struct DependencyInfo {
   pub id: usize,
-  pub children: Vec<usize>,
+  pub children: IndexSet<usize>,
   pub size: Option<u64>,
   pub media_type: Option<MediaType>,
 }
@@ -2165,6 +2173,7 @@ pub async fn get_score_handler(
 mod test {
   use hyper::Body;
   use hyper::StatusCode;
+  use indexmap::IndexSet;
   use serde_json::json;
 
   use crate::api::ApiDependency;
@@ -3631,10 +3640,11 @@ ggHohNAjhbzDaY2iBW/m3NC5dehGUP4T2GBo/cwGhg==
     assert_eq!(
       deps,
       vec![ApiDependencyGraphItem {
+        id: 0,
         dependency: super::DependencyKind::Root {
           path: "/mod.ts".to_string(),
         },
-        children: vec![],
+        children: IndexSet::new(),
         size: Some(155),
         media_type: Some("TypeScript".to_string()),
       }]
@@ -3664,21 +3674,23 @@ ggHohNAjhbzDaY2iBW/m3NC5dehGUP4T2GBo/cwGhg==
       deps,
       vec![
         ApiDependencyGraphItem {
+          id: 1,
           dependency: super::DependencyKind::Jsr {
             scope: "scope".to_string(),
             package: "foo".to_string(),
             version: "1.2.3".to_string(),
             entrypoint: super::JsrEntrypoint::Entrypoint(".".to_string())
           },
-          children: vec![],
+          children: IndexSet::new(),
           size: Some(155),
           media_type: Some("TypeScript".to_string())
         },
         ApiDependencyGraphItem {
+          id: 0,
           dependency: super::DependencyKind::Root {
             path: "/mod.ts".to_string()
           },
-          children: vec![0],
+          children: IndexSet::from([1]),
           size: Some(117),
           media_type: Some("TypeScript".to_string())
         }

api/src/api/types.rs

@@ -84,8 +84,9 @@ impl From<PublishingTask> for ApiPublishingTask {
 #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct ApiDependencyGraphItem {
+  pub id: usize,
   pub dependency: super::package::DependencyKind,
-  pub children: Vec<usize>,
+  pub children: indexmap::IndexSet<usize>,
   pub size: Option<u64>,
   pub media_type: Option<String>,
 }
@@ -103,6 +104,7 @@ impl
     ),
   ) -> Self {
     Self {
+      id: info.id,
       dependency: kind,
       children: info.children,
       size: info.size,

frontend/routes/package/(_islands)/DependencyGraph.tsx

@@ -48,16 +48,16 @@ interface JsrPackage {
   version: string;
 }
 
-export function groupDependencies(
+function groupDependencies(
   items: DependencyGraphItem[],
 ): GroupedDependencyGraphItem[] {
   const referencedBy = new Map<number, Set<number>>();
-  for (let i = 0; i < items.length; i++) {
-    for (const child of items[i].children) {
-      if (!referencedBy.has(child)) {
-        referencedBy.set(child, new Set());
+  for (const item of items) {
+    for (const childId of item.children) {
+      if (!referencedBy.has(childId)) {
+        referencedBy.set(childId, new Set());
       }
-      referencedBy.get(child)!.add(i);
+      referencedBy.get(childId)!.add(item.id);
     }
   }
 
@@ -66,16 +66,15 @@ export function groupDependencies(
     entrypoints: {
       entrypoint: string;
       isEntrypoint: boolean;
-      oldIndex: number;
+      oldId: number;
     }[];
     children: number[];
     size: number | undefined;
     mediaType: string | undefined;
-    oldIndices: number[];
+    oldIds: number[];
   }>();
 
-  for (let i = 0; i < items.length; i++) {
-    const item = items[i];
+  for (const item of items) {
     if (item.dependency.type === "jsr") {
       const groupKey =
         `${item.dependency.scope}/${item.dependency.package}@${item.dependency.version}`;
@@ -89,29 +88,28 @@ export function groupDependencies(
         children: [],
         size: undefined,
         mediaType: undefined,
-        oldIndices: [],
+        oldIds: [],
       };
       group.entrypoints.push({
         entrypoint: item.dependency.entrypoint.value,
         isEntrypoint: item.dependency.entrypoint.type == "entrypoint",
-        oldIndex: i,
+        oldId: item.id,
       });
       group.children.push(...item.children);
       if (item.size !== undefined) {
         group.size ??= 0;
         group.size += item.size;
       }
-      group.oldIndices.push(i);
+      group.oldIds.push(item.id);
       jsrGroups.set(groupKey, group);
     }
   }
 
-  const oldIndexToNewIndex = new Map<number, number>();
+  const idToIndex = new Map<number, number>();
   const placedJsrGroups = new Set<string>();
   const out: GroupedDependencyGraphItem[] = [];
 
-  for (let i = 0; i < items.length; i++) {
-    const item = items[i];
+  for (const item of items) {
     if (item.dependency.type === "jsr") {
       const groupKey =
         `${item.dependency.scope}/${item.dependency.package}@${item.dependency.version}`;
@@ -120,12 +118,11 @@ export function groupDependencies(
       if (!placedJsrGroups.has(groupKey)) {
         placedJsrGroups.add(groupKey);
 
-        const groupIndicesSet = new Set(group.oldIndices);
-        const filteredEntrypoints = group.entrypoints.filter(({ oldIndex }) => {
-          const refs = referencedBy.get(oldIndex)!;
-
+        const groupIds = new Set(group.oldIds);
+        const filteredEntrypoints = group.entrypoints.filter(({ oldId }) => {
+          const refs = referencedBy.get(oldId)!;
           for (const ref of refs) {
-            if (!groupIndicesSet.has(ref)) {
+            if (!groupIds.has(ref)) {
               return true;
             }
           }
@@ -153,13 +150,13 @@ export function groupDependencies(
           mediaType: group.mediaType,
         });
 
-        for (const oldIdx of group.oldIndices) {
-          oldIndexToNewIndex.set(oldIdx, newIndex);
+        for (const oldId of group.oldIds) {
+          idToIndex.set(oldId, newIndex);
         }
       } else {
-        oldIndexToNewIndex.set(
-          i,
-          oldIndexToNewIndex.get(jsrGroups.get(groupKey)!.oldIndices[0])!,
+        idToIndex.set(
+          item.id,
+          idToIndex.get(jsrGroups.get(groupKey)!.oldIds[0])!,
         );
       }
     } else {
@@ -169,14 +166,14 @@ export function groupDependencies(
         size: item.size,
         mediaType: item.mediaType,
       });
-      oldIndexToNewIndex.set(i, out.length - 1);
+      idToIndex.set(item.id, out.length - 1);
     }
   }
 
   for (let index = 0; index < out.length; index++) {
     const newItem = out[index];
     const remappedChildren = newItem.children
-      .map((childIdx) => oldIndexToNewIndex.get(childIdx)!)
+      .map((oldId) => idToIndex.get(oldId)!)
       .filter((childNewIdx) => childNewIdx !== index);
     newItem.children = Array.from(new Set(remappedChildren));
   }

frontend/utils/api_types.ts

@@ -302,6 +302,7 @@ export type DependencyGraphKind =
   | DependencyGraphKindError;
 
 export interface DependencyGraphItem {
+  id: number;
   dependency: DependencyGraphKind;
   children: number[];
   size: number | undefined;