From 1574516e5dce942cac7da398d6037dfe8766dc5d Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Fri, 31 Jan 2025 18:53:43 +0100 Subject: [PATCH] fix: make dependency graph more robust (#909) Fixes #898 Needed for easier debugging (script to clone a jsr package and all its dependencies locally) --- api/src/api/package.rs | 38 ++++++++----- api/src/api/types.rs | 4 +- .../package/(_islands)/DependencyGraph.tsx | 53 +++++++++---------- frontend/utils/api_types.ts | 1 + 4 files changed, 54 insertions(+), 42 deletions(-) diff --git a/api/src/api/package.rs b/api/src/api/package.rs index e81ae463..a643b9d2 100644 --- a/api/src/api/package.rs +++ b/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, exports: &'a IndexMap>, id_index: &'a mut usize, + visited: IndexSet, } 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, + pub children: IndexSet, pub size: Option, pub media_type: Option, } @@ -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()) } diff --git a/api/src/api/types.rs b/api/src/api/types.rs index 50886e07..00647e40 100644 --- a/api/src/api/types.rs +++ b/api/src/api/types.rs @@ -84,8 +84,9 @@ impl From 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, + pub children: indexmap::IndexSet, pub size: Option, pub media_type: Option, } @@ -103,6 +104,7 @@ impl ), ) -> Self { Self { + id: info.id, dependency: kind, children: info.children, size: info.size, diff --git a/frontend/routes/package/(_islands)/DependencyGraph.tsx b/frontend/routes/package/(_islands)/DependencyGraph.tsx index a3d2127f..02fc1f67 100644 --- a/frontend/routes/package/(_islands)/DependencyGraph.tsx +++ b/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>(); - 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(); + const idToIndex = new Map(); const placedJsrGroups = new Set(); 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)); } diff --git a/frontend/utils/api_types.ts b/frontend/utils/api_types.ts index 2adf36fa..598fef80 100644 --- a/frontend/utils/api_types.ts +++ b/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;