From 7998663ec883c9713548a586f49826262a8a48a7 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 6 Mar 2023 18:21:14 -0500 Subject: [PATCH 01/15] fix(npm): improve peer dependency resolution more --- cli/npm/resolution/graph.rs | 211 ++++++++++++++++++++++++++++-------- 1 file changed, 165 insertions(+), 46 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 87579dad32482d..f61b760ede372c 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -15,6 +15,7 @@ use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; use deno_graph::npm::NpmPackageNv; use deno_graph::npm::NpmPackageReq; +use deno_graph::semver::Version; use deno_graph::semver::VersionReq; use log::debug; @@ -294,7 +295,7 @@ pub struct Graph { /// Note: Uses a BTreeMap in order to create some determinism /// when creating the snapshot. root_packages: BTreeMap, - nodes_by_package_name: HashMap>, + package_name_versions: HashMap>, nodes: HashMap, resolved_node_ids: ResolvedNodeIds, // This will be set when creating from a snapshot, then @@ -463,14 +464,7 @@ impl Graph { new_seen.clone(), ); - // This condition prevents a name showing up in the peer_dependencies - // list that matches the current name. Checking just the name and - // version should be sufficient because the rest of the peer dependency - // resolutions should be the same - let is_pkg_same = child_peer.nv == npm_pkg_id.nv; - if !is_pkg_same - && seen_children_resolved_ids.insert(child_peer.clone()) - { + if seen_children_resolved_ids.insert(child_peer.clone()) { npm_pkg_id.peer_dependencies.push(child_peer); } } @@ -501,10 +495,10 @@ impl Graph { }; self - .nodes_by_package_name + .package_name_versions .entry(pkg_nv.name.clone()) .or_default() - .push(node_id); + .insert(pkg_nv.version.clone()); self.nodes.insert(node_id, node); node_id @@ -540,6 +534,8 @@ impl Graph { self.nodes.len(), ); let mut packages = HashMap::with_capacity(self.nodes.len()); + let mut packages_by_name: HashMap> = + HashMap::with_capacity(self.nodes.len()); let mut traversed_node_ids = HashSet::with_capacity(self.nodes.len()); let mut pending = VecDeque::new(); for root_id in self.root_packages.values() { @@ -550,6 +546,12 @@ impl Graph { while let Some(node_id) = pending.pop_front() { let node = self.nodes.get(&node_id).unwrap(); let resolved_id = packages_to_resolved_id.get(&node_id).unwrap(); + + packages_by_name + .entry(resolved_id.nv.name.clone()) + .or_default() + .push(resolved_id.clone()); + // todo(dsherret): grab this from the dep entry cache, which should have it let dist = api .package_version_info(&resolved_id.nv) @@ -594,15 +596,9 @@ impl Graph { (id, packages_to_resolved_id.get(&node_id).unwrap().clone()) }) .collect(), - packages_by_name: self - .nodes_by_package_name + packages_by_name: packages_by_name .into_iter() - .map(|(name, ids)| { - let mut ids = ids - .into_iter() - .filter(|id| traversed_node_ids.contains(id)) - .map(|id| packages_to_resolved_id.get(&id).unwrap().clone()) - .collect::>(); + .map(|(name, mut ids)| { ids.sort(); ids.dedup(); (name, ids) @@ -853,19 +849,10 @@ impl<'a> GraphDependencyResolver<'a> { package_info, self .graph - .nodes_by_package_name + .package_name_versions .entry(package_info.name.clone()) .or_default() - .iter() - .map(|node_id| { - &self - .graph - .resolved_node_ids - .get(*node_id) - .unwrap() - .nv - .version - }), + .iter(), )?; let resolved_id = ResolvedId { nv: NpmPackageNv { @@ -910,7 +897,7 @@ impl<'a> GraphDependencyResolver<'a> { while !self.pending_unresolved_nodes.is_empty() { // now go down through the dependencies by tree depth while let Some(graph_path) = self.pending_unresolved_nodes.pop_front() { - let (pkg_id, deps) = { + let (pkg_nv, deps) = { let node_id = graph_path.node_id(); if self.graph.nodes.get(&node_id).unwrap().no_peers { // We can skip as there's no reason to analyze this graph segment further @@ -960,10 +947,23 @@ impl<'a> GraphDependencyResolver<'a> { match dep.kind { NpmDependencyEntryKind::Dep => { - // todo(dsherret): look into skipping dependency analysis if - // it was done previously again - let child_id = - self.analyze_dependency(dep, &package_info, &graph_path)?; + let parent_id = graph_path.node_id(); + let node = self.graph.nodes.get(&parent_id).unwrap(); + let child_id = match node.children.get(&dep.bare_specifier) { + Some(child_id) => { + // skip over this dependency because it was previously resolved + let child_id = *child_id; + self.try_add_pending_unresolved_node( + &graph_path, + child_id, + &dep.bare_specifier, + ); + child_id + } + None => { + self.analyze_dependency(dep, &package_info, &graph_path)? + } + }; if !found_peer { found_peer = !self.graph.borrow_node_mut(child_id).no_peers; @@ -993,7 +993,7 @@ impl<'a> GraphDependencyResolver<'a> { match maybe_new_id { Some(new_id) => { if let Some(unresolved_optional_peers) = - self.unresolved_optional_peers.remove(&pkg_id) + self.unresolved_optional_peers.remove(&pkg_nv) { for optional_peer in unresolved_optional_peers { let peer_parent = GraphPathNodeOrRoot::Node( @@ -1012,7 +1012,7 @@ impl<'a> GraphDependencyResolver<'a> { // store this for later if it's resolved for this version self .unresolved_optional_peers - .entry(pkg_id.clone()) + .entry(pkg_nv.clone()) .or_default() .push(UnresolvedOptionalPeer { specifier: dep.bare_specifier.clone(), @@ -1174,7 +1174,7 @@ impl<'a> GraphDependencyResolver<'a> { mut peer_dep_id: NodeId, ) { debug_assert!(!path.is_empty()); - let peer_dep_pkg_id = self + let peer_dep_pkg_nv = self .graph .resolved_node_ids .get(peer_dep_id) @@ -1184,8 +1184,9 @@ impl<'a> GraphDependencyResolver<'a> { let peer_dep = ResolvedIdPeerDep::ParentReference { parent: peer_dep_parent, - child_pkg_nv: peer_dep_pkg_id, + child_pkg_nv: peer_dep_pkg_nv.clone(), }; + let mut found_in_ancestor = false; for graph_path_node in path.iter().rev() { let old_node_id = graph_path_node.node_id(); let old_resolved_id = self @@ -1196,13 +1197,16 @@ impl<'a> GraphDependencyResolver<'a> { .clone(); let mut new_resolved_id = old_resolved_id.clone(); - new_resolved_id.push_peer_dep(peer_dep.clone()); + if new_resolved_id.nv != peer_dep_pkg_nv { + new_resolved_id.push_peer_dep(peer_dep.clone()); + } let (created, new_node_id) = self.graph.get_or_create_for_id(&new_resolved_id); // this will occur when the peer dependency is in an ancestor if old_node_id == peer_dep_id { peer_dep_id = new_node_id; + found_in_ancestor = true; } if created { @@ -1243,12 +1247,15 @@ impl<'a> GraphDependencyResolver<'a> { parent_node_id, ); - // mark the peer dependency to be analyzed - self.try_add_pending_unresolved_node( - bottom_node, - peer_dep_id, - peer_dep_specifier, - ); + // mark the peer dependency to be analyzed, but only bother + // if it wasn't found in an ancestor + if !found_in_ancestor { + self.try_add_pending_unresolved_node( + bottom_node, + peer_dep_id, + peer_dep_specifier, + ); + } debug!( "Resolved peer dependency for {} in {} to {}", @@ -3377,6 +3384,118 @@ mod test { ); } + #[tokio::test] + async fn resolve_peer_deps_in_ancestor_root() { + let api = TestNpmRegistryApiInner::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "1.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-a", "1")); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; + assert_eq!( + packages, + vec![ + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0_package-a@1.0.0") + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-b@1.0.0_package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized("package-c@1.0.0_package-a@1.0.0") + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-c@1.0.0_package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-a".to_string(), + NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + )]), + dist: Default::default(), + }, + ] + ); + assert_eq!( + package_reqs, + vec![("package-a@1.0.0".to_string(), "package-a@1.0.0".to_string())] + ); + } + + #[tokio::test] + async fn resolve_peer_deps_in_ancestor_non_root() { + let api = TestNpmRegistryApiInner::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "1.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-b", "1")); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; + assert_eq!( + packages, + vec![ + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + ),]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-c@1.0.0_package-b@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + )]), + dist: Default::default(), + }, + ] + ); + assert_eq!( + package_reqs, + vec![("package-a@1.0.0".to_string(), "package-a@1.0.0".to_string())] + ); + } + #[tokio::test] async fn nested_deps_same_peer_dep_ancestor() { let api = TestNpmRegistryApiInner::default(); From 124dfa550d0cc41c8df4e3a5b055b0a2c5008a22 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 6 Mar 2023 19:32:27 -0500 Subject: [PATCH 02/15] Add failing test. --- cli/npm/resolution/graph.rs | 95 ++++++++++++++++++++++++++++++++----- 1 file changed, 83 insertions(+), 12 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index f61b760ede372c..ff1c068eccbc88 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -831,7 +831,7 @@ impl<'a> GraphDependencyResolver<'a> { .nv .clone(); let new_path = match path.with_id(node_id, specifier, node_nv) { - Some(visited_versions) => visited_versions, + Some(path) => path, None => return, // circular, don't visit this node }; self.pending_unresolved_nodes.push_back(new_path); @@ -900,10 +900,7 @@ impl<'a> GraphDependencyResolver<'a> { let (pkg_nv, deps) = { let node_id = graph_path.node_id(); if self.graph.nodes.get(&node_id).unwrap().no_peers { - // We can skip as there's no reason to analyze this graph segment further - // Note that we don't need to count parent references here because that's - // only necessary for graph segments that could potentially have peer - // dependencies within them. + // We can skip as there's no reason to analyze this graph segment further. continue; } @@ -1249,13 +1246,13 @@ impl<'a> GraphDependencyResolver<'a> { // mark the peer dependency to be analyzed, but only bother // if it wasn't found in an ancestor - if !found_in_ancestor { - self.try_add_pending_unresolved_node( - bottom_node, - peer_dep_id, - peer_dep_specifier, - ); - } + //if !found_in_ancestor { + self.try_add_pending_unresolved_node( + bottom_node, + peer_dep_id, + peer_dep_specifier, + ); + //} debug!( "Resolved peer dependency for {} in {} to {}", @@ -3733,6 +3730,80 @@ mod test { ); } + #[tokio::test] + async fn resolve_dep_with_peer_deps_circular() { + // a -> b -> c -> d -> c where c has a peer dependency on b + let api = TestNpmRegistryApiInner::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "1.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.ensure_package_version("package-d", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); + api.add_dependency(("package-c", "1.0.0"), ("package-d", "1")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-d", "1.0.0"), ("package-c", "1")); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; + assert_eq!( + packages, + vec![ + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + ),]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-c@1.0.0_package-b@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([ + ( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + ), + ( + "package-d".to_string(), + NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + ) + ]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") + .unwrap(), + )]), + dist: Default::default(), + }, + ] + ); + assert_eq!( + package_reqs, + vec![("package-a@1.0.0".to_string(), "package-a@1.0.0".to_string())] + ); + } + async fn run_resolver_and_get_output( api: TestNpmRegistryApiInner, reqs: Vec<&str>, From 4c5e285ef8da5d8dc9e413f86baf582ec421abbf Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 6 Mar 2023 20:37:00 -0500 Subject: [PATCH 03/15] Correct test. --- cli/npm/resolution/graph.rs | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index ff1c068eccbc88..da459859d8526f 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -351,7 +351,7 @@ impl Graph { packages, created_package_ids, )?; - graph.set_child_parent_node(name, child_node_id, node_id); + graph.set_child_of_parent_node(name, child_node_id, node_id); } Ok(node_id) } @@ -508,7 +508,7 @@ impl Graph { self.nodes.get_mut(&node_id).unwrap() } - fn set_child_parent_node( + fn set_child_of_parent_node( &mut self, specifier: &str, child_id: NodeId, @@ -800,7 +800,7 @@ impl<'a> GraphDependencyResolver<'a> { // just ignore adding these as dependencies because this is likely a mistake // in the package. if node_id != parent_id { - self.graph.set_child_parent_node( + self.graph.set_child_of_parent_node( &entry.bare_specifier, node_id, parent_id, @@ -1211,9 +1211,11 @@ impl<'a> GraphDependencyResolver<'a> { self.graph.borrow_node_mut(old_node_id).children.clone(); // copy over the old children to this new one for (specifier, child_id) in &old_children { - self - .graph - .set_child_parent_node(specifier, *child_id, new_node_id); + self.graph.set_child_of_parent_node( + specifier, + *child_id, + new_node_id, + ); } } @@ -1238,7 +1240,7 @@ impl<'a> GraphDependencyResolver<'a> { // now set the peer dependency let bottom_node = path.first().unwrap(); let parent_node_id = bottom_node.node_id(); - self.graph.set_child_parent_node( + self.graph.set_child_of_parent_node( peer_dep_specifier, peer_dep_id, parent_node_id, @@ -3781,13 +3783,17 @@ mod test { ), ( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + NpmPackageId::from_serialized("package-d@1.0.0_package-b@1.0.0") + .unwrap(), ) ]), dist: Default::default(), }, NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + pkg_id: NpmPackageId::from_serialized( + "package-d@1.0.0_package-b@1.0.0" + ) + .unwrap(), copy_index: 0, dependencies: HashMap::from([( "package-c".to_string(), From 49f622714406f272307be6e1944b05bbb038e590 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 12:57:46 -0500 Subject: [PATCH 04/15] Add concept of "linked circular descendants" --- cli/npm/resolution/graph.rs | 344 +++++++++++++++++++++++------------- 1 file changed, 223 insertions(+), 121 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index da459859d8526f..e15abe4ff3aedb 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -116,14 +116,15 @@ impl ResolvedId { hasher.finish() } - pub fn push_peer_dep(&mut self, peer_dep: ResolvedIdPeerDep) { + pub fn push_peer_dep(&mut self, peer_dep: ResolvedIdPeerDep) -> bool { let new_hash = peer_dep.current_state_hash(); for dep in &self.peer_dependencies { if new_hash == dep.current_state_hash() { - return; + return false; } } self.peer_dependencies.push(peer_dep); + true } } @@ -192,14 +193,16 @@ enum GraphPathNodeOrRoot { /// the dependency resolution. The graph tries to share duplicate package /// information and we try to avoid traversing parts of the graph that we know /// are resolved. -#[derive(Clone)] struct GraphPath { previous_node: Option, node_id_ref: NodeIdRef, - // todo(dsherret): I think we might be able to get rid of specifier and - // node version here, but I added them for extra protection for the time being. specifier: String, + // we could consider not storing this here and instead reference the resolved + // nodes, but we should performance profile this code first nv: NpmPackageNv, + /// Descendants in the path that circularly link to an ancestor in a child.These + /// descendants should be kept up to date and always point to this node. + linked_circular_descendants: Mutex>>, } impl GraphPath { @@ -210,6 +213,7 @@ impl GraphPath { // use an empty specifier specifier: "".to_string(), nv, + linked_circular_descendants: Default::default(), }) } @@ -228,38 +232,47 @@ impl GraphPath { pub fn with_id( self: &Arc, node_id: NodeId, - specifier: &str, + specifier: String, nv: NpmPackageNv, - ) -> Option> { - if self.has_visited(&nv) { - None - } else { - Some(Arc::new(Self { - previous_node: Some(GraphPathNodeOrRoot::Node(self.clone())), - node_id_ref: NodeIdRef::new(node_id), - specifier: specifier.to_string(), - nv, - })) - } + ) -> Arc { + Arc::new(Self { + previous_node: Some(GraphPathNodeOrRoot::Node(self.clone())), + node_id_ref: NodeIdRef::new(node_id), + specifier, + nv, + linked_circular_descendants: Default::default(), + }) } - /// Each time an identifier is added, we do a check to ensure - /// that we haven't previously visited this node. I suspect this - /// might be a little slow since it has to go up through the ancestors, - /// so some optimizations could be made here in the future. - pub fn has_visited(self: &Arc, nv: &NpmPackageNv) -> bool { - if self.nv == *nv { - return true; - } + /// Gets if there is an ancestor with the same name & version along this path. + pub fn find_ancestor(&self, nv: &NpmPackageNv) -> Option> { let mut maybe_next_node = self.previous_node.as_ref(); while let Some(GraphPathNodeOrRoot::Node(next_node)) = maybe_next_node { // we've visited this before, so stop if next_node.nv == *nv { - return true; + return Some(next_node.clone()); + } + maybe_next_node = next_node.previous_node.as_ref(); + } + None + } + + /// Gets the bottom-up path to the ancestor, not including the current node. + pub fn get_path_to_ancestor( + &self, + ancestor_node_id: NodeId, + ) -> Vec<&Arc> { + let mut path = Vec::new(); + let mut maybe_next_node = self.previous_node.as_ref(); + while let Some(GraphPathNodeOrRoot::Node(next_node)) = maybe_next_node { + path.push(next_node); + if next_node.node_id() == ancestor_node_id { + break; } maybe_next_node = next_node.previous_node.as_ref(); } - false + debug_assert!(maybe_next_node.is_some()); + path } pub fn ancestors(&self) -> GraphPathAncestorIterator { @@ -786,11 +799,11 @@ impl<'a> GraphDependencyResolver<'a> { &mut self, entry: &NpmDependencyEntry, package_info: &NpmPackageInfo, - graph_path: &Arc, + parent_path: &Arc, ) -> Result { debug_assert_eq!(entry.kind, NpmDependencyEntryKind::Dep); - let parent_id = graph_path.node_id(); - let (_, node_id) = self.resolve_node_from_info( + let parent_id = parent_path.node_id(); + let (child_nv, child_id) = self.resolve_node_from_info( &entry.name, &entry.version_req, package_info, @@ -799,42 +812,27 @@ impl<'a> GraphDependencyResolver<'a> { // Some packages may resolves to themselves as a dependency. If this occurs, // just ignore adding these as dependencies because this is likely a mistake // in the package. - if node_id != parent_id { + if child_id != parent_id { self.graph.set_child_of_parent_node( &entry.bare_specifier, - node_id, + child_id, parent_id, ); - self.try_add_pending_unresolved_node( - graph_path, - node_id, - &entry.bare_specifier, - ); - } - Ok(node_id) - } - fn try_add_pending_unresolved_node( - &mut self, - path: &Arc, - node_id: NodeId, - specifier: &str, - ) { - if self.graph.nodes.get(&node_id).unwrap().no_peers { - return; // skip, no need to analyze this again + let maybe_ancestor = parent_path.find_ancestor(&child_nv); + let new_path = parent_path.with_id( + child_id, + entry.bare_specifier.to_string(), + child_nv, + ); + if let Some(ancestor) = maybe_ancestor { + // this node is circular, so we link it to the ancestor + self.add_linked_circular_descendant(&ancestor, new_path); + } else { + self.pending_unresolved_nodes.push_back(new_path); + } } - let node_nv = self - .graph - .resolved_node_ids - .get(node_id) - .unwrap() - .nv - .clone(); - let new_path = match path.with_id(node_id, specifier, node_nv) { - Some(path) => path, - None => return, // circular, don't visit this node - }; - self.pending_unresolved_nodes.push_back(new_path); + Ok(child_id) } fn resolve_node_from_info( @@ -896,9 +894,9 @@ impl<'a> GraphDependencyResolver<'a> { pub async fn resolve_pending(&mut self) -> Result<(), AnyError> { while !self.pending_unresolved_nodes.is_empty() { // now go down through the dependencies by tree depth - while let Some(graph_path) = self.pending_unresolved_nodes.pop_front() { - let (pkg_nv, deps) = { - let node_id = graph_path.node_id(); + while let Some(parent_path) = self.pending_unresolved_nodes.pop_front() { + let (parent_nv, child_deps) = { + let node_id = parent_path.node_id(); if self.graph.nodes.get(&node_id).unwrap().no_peers { // We can skip as there's no reason to analyze this graph segment further. continue; @@ -932,33 +930,49 @@ impl<'a> GraphDependencyResolver<'a> { self .api .cache_in_parallel({ - deps.iter().map(|dep| dep.name.clone()).collect() + child_deps.iter().map(|dep| dep.name.clone()).collect() }) .await?; // resolve the dependencies let mut found_peer = false; - for dep in deps.iter() { + for dep in child_deps.iter() { let package_info = self.api.package_info(&dep.name).await?; match dep.kind { NpmDependencyEntryKind::Dep => { - let parent_id = graph_path.node_id(); + let parent_id = parent_path.node_id(); let node = self.graph.nodes.get(&parent_id).unwrap(); let child_id = match node.children.get(&dep.bare_specifier) { Some(child_id) => { - // skip over this dependency because it was previously resolved + // this dependency was previously analyzed by another path + // so we don't attempt to resolve the version again let child_id = *child_id; - self.try_add_pending_unresolved_node( - &graph_path, + let child_nv = self + .graph + .resolved_node_ids + .get(child_id) + .unwrap() + .nv + .clone(); + let maybe_ancestor = parent_path.find_ancestor(&child_nv); + let child_path = parent_path.with_id( child_id, - &dep.bare_specifier, + dep.bare_specifier.clone(), + child_nv, ); + if let Some(ancestor) = maybe_ancestor { + // it's circular, so mark it as such in the ancestor + self.add_linked_circular_descendant(&ancestor, child_path); + } else { + // mark the child as pending + self.pending_unresolved_nodes.push_back(child_path); + } child_id } None => { - self.analyze_dependency(dep, &package_info, &graph_path)? + self.analyze_dependency(dep, &package_info, &parent_path)? } }; @@ -976,7 +990,7 @@ impl<'a> GraphDependencyResolver<'a> { &dep.bare_specifier, dep, &package_info, - &graph_path, + &parent_path, )?; // For optional dependencies, we want to resolve them if any future @@ -990,14 +1004,14 @@ impl<'a> GraphDependencyResolver<'a> { match maybe_new_id { Some(new_id) => { if let Some(unresolved_optional_peers) = - self.unresolved_optional_peers.remove(&pkg_nv) + self.unresolved_optional_peers.remove(&parent_nv) { for optional_peer in unresolved_optional_peers { let peer_parent = GraphPathNodeOrRoot::Node( optional_peer.graph_path.clone(), ); self.set_new_peer_dep( - vec![&optional_peer.graph_path], + &[&optional_peer.graph_path], peer_parent, &optional_peer.specifier, new_id, @@ -1009,11 +1023,11 @@ impl<'a> GraphDependencyResolver<'a> { // store this for later if it's resolved for this version self .unresolved_optional_peers - .entry(pkg_nv.clone()) + .entry(parent_nv.clone()) .or_default() .push(UnresolvedOptionalPeer { specifier: dep.bare_specifier.clone(), - graph_path: graph_path.clone(), + graph_path: parent_path.clone(), }); } } @@ -1023,7 +1037,7 @@ impl<'a> GraphDependencyResolver<'a> { } if !found_peer { - self.graph.borrow_node_mut(graph_path.node_id()).no_peers = true; + self.graph.borrow_node_mut(parent_path.node_id()).no_peers = true; } } } @@ -1055,7 +1069,7 @@ impl<'a> GraphDependencyResolver<'a> { if let Some((peer_parent, peer_dep_id)) = maybe_peer_dep { // this will always have an ancestor because we're not at the root - self.set_new_peer_dep(path, peer_parent, specifier, peer_dep_id); + self.set_new_peer_dep(&path, peer_parent, specifier, peer_dep_id); return Ok(Some(peer_dep_id)); } } @@ -1073,7 +1087,7 @@ impl<'a> GraphDependencyResolver<'a> { )?; if let Some((parent, peer_dep_id)) = maybe_peer_dep { // this will always have an ancestor because we're not at the root - self.set_new_peer_dep(path, parent, specifier, peer_dep_id); + self.set_new_peer_dep(&path, parent, specifier, peer_dep_id); return Ok(Some(peer_dep_id)); } } @@ -1089,7 +1103,7 @@ impl<'a> GraphDependencyResolver<'a> { .map(|(pkg_id, id)| (*id, pkg_id)), )? { let peer_parent = GraphPathNodeOrRoot::Root(root_pkg_id.clone()); - self.set_new_peer_dep(path, peer_parent, specifier, child_id); + self.set_new_peer_dep(&path, peer_parent, specifier, child_id); return Ok(Some(child_id)); } } @@ -1110,12 +1124,7 @@ impl<'a> GraphDependencyResolver<'a> { Some(parent_id), )?; let peer_parent = GraphPathNodeOrRoot::Node(ancestor_path.clone()); - self.set_new_peer_dep( - vec![ancestor_path], - peer_parent, - specifier, - node_id, - ); + self.set_new_peer_dep(&[ancestor_path], peer_parent, specifier, node_id); Ok(Some(node_id)) } else { Ok(None) @@ -1162,28 +1171,17 @@ impl<'a> GraphDependencyResolver<'a> { } } - fn set_new_peer_dep( + fn add_peer_dep_to_path( &mut self, // path from the node above the resolved dep to just above the peer dep - path: Vec<&Arc>, - peer_dep_parent: GraphPathNodeOrRoot, - peer_dep_specifier: &str, - mut peer_dep_id: NodeId, - ) { + path: &[&Arc], + peer_dep: &ResolvedIdPeerDep, + peer_dep_nv: &NpmPackageNv, + peer_dep_id: Option, + ) -> Option> { debug_assert!(!path.is_empty()); - let peer_dep_pkg_nv = self - .graph - .resolved_node_ids - .get(peer_dep_id) - .unwrap() - .nv - .clone(); - let peer_dep = ResolvedIdPeerDep::ParentReference { - parent: peer_dep_parent, - child_pkg_nv: peer_dep_pkg_nv.clone(), - }; - let mut found_in_ancestor = false; + let mut found_ancestor_node = None; for graph_path_node in path.iter().rev() { let old_node_id = graph_path_node.node_id(); let old_resolved_id = self @@ -1193,19 +1191,24 @@ impl<'a> GraphDependencyResolver<'a> { .unwrap() .clone(); + // todo: get rid of passing this in? + let is_current_ancestor_peer_dep = Some(old_node_id) == peer_dep_id; + if is_current_ancestor_peer_dep { + debug_assert_eq!(old_resolved_id.nv, *peer_dep_nv); + found_ancestor_node = Some((*graph_path_node).clone()); + continue; + } else if old_resolved_id.nv == *peer_dep_nv { + continue; + } + let mut new_resolved_id = old_resolved_id.clone(); - if new_resolved_id.nv != peer_dep_pkg_nv { - new_resolved_id.push_peer_dep(peer_dep.clone()); + if !new_resolved_id.push_peer_dep(peer_dep.clone()) { + continue; // nothing to change } + let (created, new_node_id) = self.graph.get_or_create_for_id(&new_resolved_id); - // this will occur when the peer dependency is in an ancestor - if old_node_id == peer_dep_id { - peer_dep_id = new_node_id; - found_in_ancestor = true; - } - if created { let old_children = self.graph.borrow_node_mut(old_node_id).children.clone(); @@ -1221,6 +1224,21 @@ impl<'a> GraphDependencyResolver<'a> { debug_assert_eq!(graph_path_node.node_id(), old_node_id); graph_path_node.change_id(new_node_id); + let circular_descendants = + graph_path_node.linked_circular_descendants.lock().clone(); + for descendant in circular_descendants { + let path = descendant.get_path_to_ancestor(new_node_id); + self.add_peer_dep_to_path(&path, peer_dep, peer_dep_nv, None); + descendant.change_id(new_node_id); + + // update the parent to point to this new node id + let parent_node_id = path[0].node_id(); + self.graph.set_child_of_parent_node( + descendant.specifier(), + descendant.node_id(), + parent_node_id, + ); + } // update the previous parent to have this as its child match graph_path_node.previous_node.as_ref().unwrap() { @@ -1237,6 +1255,38 @@ impl<'a> GraphDependencyResolver<'a> { } } + found_ancestor_node + } + + fn set_new_peer_dep( + &mut self, + // path from the node above the resolved dep to just above the peer dep + path: &[&Arc], + peer_dep_parent: GraphPathNodeOrRoot, + peer_dep_specifier: &str, + peer_dep_id: NodeId, + ) { + debug_assert!(!path.is_empty()); + let peer_dep_nv = self + .graph + .resolved_node_ids + .get(peer_dep_id) + .unwrap() + .nv + .clone(); + + let peer_dep = ResolvedIdPeerDep::ParentReference { + parent: peer_dep_parent, + child_pkg_nv: peer_dep_nv.clone(), + }; + + let found_ancestor_node = self.add_peer_dep_to_path( + path, + &peer_dep, + &peer_dep_nv, + Some(peer_dep_id), + ); + // now set the peer dependency let bottom_node = path.first().unwrap(); let parent_node_id = bottom_node.node_id(); @@ -1246,23 +1296,75 @@ impl<'a> GraphDependencyResolver<'a> { parent_node_id, ); - // mark the peer dependency to be analyzed, but only bother - // if it wasn't found in an ancestor - //if !found_in_ancestor { - self.try_add_pending_unresolved_node( - bottom_node, + // queue next step + let new_path = bottom_node.with_id( peer_dep_id, - peer_dep_specifier, - ); - //} + peer_dep_specifier.to_string(), + peer_dep_nv, + ); + if let Some(ancestor_node) = found_ancestor_node { + // it's circular, so link this in step with the ancestor node + ancestor_node + .linked_circular_descendants + .lock() + .push(new_path); + } else { + // mark the peer dep as needing to be analyzed + self.pending_unresolved_nodes.push_back(new_path); + } debug!( "Resolved peer dependency for {} in {} to {}", peer_dep_specifier, - &self.graph.get_npm_pkg_id(parent_node_id).as_serialized(), + &self + .graph + .get_npm_pkg_id(bottom_node.node_id()) + .as_serialized(), &self.graph.get_npm_pkg_id(peer_dep_id).as_serialized(), ); } + + fn add_linked_circular_descendant( + &mut self, + ancestor: &Arc, + descendant: Arc, + ) { + let ancestor_node_id = ancestor.node_id(); + let path = descendant.get_path_to_ancestor(ancestor_node_id); + + let ancestor_resolved_id = self + .graph + .resolved_node_ids + .get(ancestor_node_id) + .unwrap() + .clone(); + + for peer_dep in ancestor_resolved_id.peer_dependencies { + let peer_dep_nv = match &peer_dep { + ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { + child_pkg_nv.clone() + } + ResolvedIdPeerDep::SnapshotNodeId(node_id) => self + .graph + .resolved_node_ids + .get(*node_id) + .unwrap() + .nv + .clone(), + }; + self.add_peer_dep_to_path(&path, &peer_dep, &peer_dep_nv, None); + } + + // todo(THIS PR): can this be removed? + let parent_node_id = path[0].node_id(); + self.graph.set_child_of_parent_node( + descendant.specifier(), + descendant.node_id(), + parent_node_id, + ); + + ancestor.linked_circular_descendants.lock().push(descendant); + } } fn find_matching_child<'a>( From 63d76f297d35ee984845db6908181f123b0f2948 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 13:09:42 -0500 Subject: [PATCH 05/15] Working test. --- cli/npm/resolution/graph.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index e15abe4ff3aedb..e5eba604ffcd9b 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -803,7 +803,7 @@ impl<'a> GraphDependencyResolver<'a> { ) -> Result { debug_assert_eq!(entry.kind, NpmDependencyEntryKind::Dep); let parent_id = parent_path.node_id(); - let (child_nv, child_id) = self.resolve_node_from_info( + let (child_nv, mut child_id) = self.resolve_node_from_info( &entry.name, &entry.version_req, package_info, @@ -813,13 +813,17 @@ impl<'a> GraphDependencyResolver<'a> { // just ignore adding these as dependencies because this is likely a mistake // in the package. if child_id != parent_id { + let maybe_ancestor = parent_path.find_ancestor(&child_nv); + if let Some(ancestor) = &maybe_ancestor { + child_id = ancestor.node_id(); + } + self.graph.set_child_of_parent_node( &entry.bare_specifier, child_id, parent_id, ); - let maybe_ancestor = parent_path.find_ancestor(&child_nv); let new_path = parent_path.with_id( child_id, entry.bare_specifier.to_string(), From 8cdaa289842d4f08fb55ba8bbf9882ba0b056bd6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 13:15:03 -0500 Subject: [PATCH 06/15] Resolve todo --- cli/npm/resolution/graph.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index e5eba604ffcd9b..f0d360db31b7aa 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -818,12 +818,6 @@ impl<'a> GraphDependencyResolver<'a> { child_id = ancestor.node_id(); } - self.graph.set_child_of_parent_node( - &entry.bare_specifier, - child_id, - parent_id, - ); - let new_path = parent_path.with_id( child_id, entry.bare_specifier.to_string(), @@ -833,6 +827,11 @@ impl<'a> GraphDependencyResolver<'a> { // this node is circular, so we link it to the ancestor self.add_linked_circular_descendant(&ancestor, new_path); } else { + self.graph.set_child_of_parent_node( + &entry.bare_specifier, + child_id, + parent_id, + ); self.pending_unresolved_nodes.push_back(new_path); } } @@ -1359,7 +1358,6 @@ impl<'a> GraphDependencyResolver<'a> { self.add_peer_dep_to_path(&path, &peer_dep, &peer_dep_nv, None); } - // todo(THIS PR): can this be removed? let parent_node_id = path[0].node_id(); self.graph.set_child_of_parent_node( descendant.specifier(), From df33830cff70a46144f127118d9df3f42b3a034f Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 13:38:14 -0500 Subject: [PATCH 07/15] Faster to reduce these clones. --- .gitignore | 1 + cli/npm/resolution/graph.rs | 118 ++++++++++++++++++++---------------- 2 files changed, 68 insertions(+), 51 deletions(-) diff --git a/.gitignore b/.gitignore index 19e60deba458e8..59166931a7335b 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ /.cargo_home/ /.idea/ +/.vs/ /.vscode/ gclient_config.py_entries /gh-pages/ diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index f0d360db31b7aa..3438fc39ecbc1a 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -62,7 +62,7 @@ enum ResolvedIdPeerDep { /// change from under it. ParentReference { parent: GraphPathNodeOrRoot, - child_pkg_nv: NpmPackageNv, + child_pkg_nv: Arc, }, /// A node that was created during snapshotting and is not being used in any path. SnapshotNodeId(NodeId), @@ -98,7 +98,7 @@ impl ResolvedIdPeerDep { /// will become fully resolved to an `NpmPackageId`. #[derive(Clone)] struct ResolvedId { - nv: NpmPackageNv, + nv: Arc, peer_dependencies: Vec, } @@ -186,7 +186,7 @@ impl NodeIdRef { #[derive(Clone)] enum GraphPathNodeOrRoot { Node(Arc), - Root(NpmPackageNv), + Root(Arc), } /// Path through the graph that represents a traversal through the graph doing @@ -199,14 +199,14 @@ struct GraphPath { specifier: String, // we could consider not storing this here and instead reference the resolved // nodes, but we should performance profile this code first - nv: NpmPackageNv, + nv: Arc, /// Descendants in the path that circularly link to an ancestor in a child.These /// descendants should be kept up to date and always point to this node. linked_circular_descendants: Mutex>>, } impl GraphPath { - pub fn for_root(node_id: NodeId, nv: NpmPackageNv) -> Arc { + pub fn for_root(node_id: NodeId, nv: Arc) -> Arc { Arc::new(Self { previous_node: Some(GraphPathNodeOrRoot::Root(nv.clone())), node_id_ref: NodeIdRef::new(node_id), @@ -233,7 +233,7 @@ impl GraphPath { self: &Arc, node_id: NodeId, specifier: String, - nv: NpmPackageNv, + nv: Arc, ) -> Arc { Arc::new(Self { previous_node: Some(GraphPathNodeOrRoot::Node(self.clone())), @@ -249,7 +249,7 @@ impl GraphPath { let mut maybe_next_node = self.previous_node.as_ref(); while let Some(GraphPathNodeOrRoot::Node(next_node)) = maybe_next_node { // we've visited this before, so stop - if next_node.nv == *nv { + if *next_node.nv == *nv { return Some(next_node.clone()); } maybe_next_node = next_node.previous_node.as_ref(); @@ -303,11 +303,11 @@ impl<'a> Iterator for GraphPathAncestorIterator<'a> { #[derive(Default)] pub struct Graph { /// Each requirement is mapped to a specific name and version. - package_reqs: HashMap, + package_reqs: HashMap>, /// Then each name and version is mapped to an exact node id. /// Note: Uses a BTreeMap in order to create some determinism /// when creating the snapshot. - root_packages: BTreeMap, + root_packages: BTreeMap, NodeId>, package_name_versions: HashMap>, nodes: HashMap, resolved_node_ids: ResolvedNodeIds, @@ -315,7 +315,7 @@ pub struct Graph { // inform the final snapshot creation. packages_to_copy_index: HashMap, /// Packages that the resolver should resolve first. - pending_unresolved_packages: Vec, + pending_unresolved_packages: Vec>, } impl Graph { @@ -324,18 +324,18 @@ impl Graph { ) -> Result { fn get_or_create_graph_node( graph: &mut Graph, - resolved_id: &NpmPackageId, + pkg_id: &NpmPackageId, packages: &HashMap, created_package_ids: &mut HashMap, ) -> Result { - if let Some(id) = created_package_ids.get(resolved_id) { + if let Some(id) = created_package_ids.get(pkg_id) { return Ok(*id); } - let node_id = graph.create_node(&resolved_id.nv); - created_package_ids.insert(resolved_id.clone(), node_id); + let node_id = graph.create_node(&pkg_id.nv); + created_package_ids.insert(pkg_id.clone(), node_id); - let peer_dep_ids = resolved_id + let peer_dep_ids = pkg_id .peer_dependencies .iter() .map(|peer_dep| { @@ -348,14 +348,14 @@ impl Graph { }) .collect::, AnyError>>()?; let graph_resolved_id = ResolvedId { - nv: resolved_id.nv.clone(), + nv: Arc::new(pkg_id.nv.clone()), peer_dependencies: peer_dep_ids, }; graph.resolved_node_ids.set(node_id, graph_resolved_id); - let resolution = match packages.get(resolved_id) { + let resolution = match packages.get(pkg_id) { Some(resolved_id) => resolved_id, // maybe the user messed around with the lockfile - None => bail!("not found package: {}", resolved_id.as_serialized()), + None => bail!("not found package: {}", pkg_id.as_serialized()), }; for (name, child_id) in &resolution.dependencies { let child_node_id = get_or_create_graph_node( @@ -377,8 +377,16 @@ impl Graph { .iter() .map(|(id, p)| (id.clone(), p.copy_index)) .collect(), - package_reqs: snapshot.package_reqs, - pending_unresolved_packages: snapshot.pending_unresolved_packages, + package_reqs: snapshot + .package_reqs + .into_iter() + .map(|(k, v)| (k, Arc::new(v))) + .collect(), + pending_unresolved_packages: snapshot + .pending_unresolved_packages + .into_iter() + .map(Arc::new) + .collect(), ..Default::default() }; let mut created_package_ids = @@ -390,12 +398,12 @@ impl Graph { &snapshot.packages, &mut created_package_ids, )?; - graph.root_packages.insert(id, node_id); + graph.root_packages.insert(Arc::new(id), node_id); } Ok(graph) } - pub fn take_pending_unresolved(&mut self) -> Vec { + pub fn take_pending_unresolved(&mut self) -> Vec> { std::mem::take(&mut self.pending_unresolved_packages) } @@ -419,12 +427,12 @@ impl Graph { ) -> NpmPackageId { if resolved_id.peer_dependencies.is_empty() { NpmPackageId { - nv: resolved_id.nv.clone(), + nv: (*resolved_id.nv).clone(), peer_dependencies: Vec::new(), } } else { let mut npm_pkg_id = NpmPackageId { - nv: resolved_id.nv.clone(), + nv: (*resolved_id.nv).clone(), peer_dependencies: Vec::with_capacity( resolved_id.peer_dependencies.len(), ), @@ -605,8 +613,11 @@ impl Graph { root_packages: self .root_packages .into_iter() - .map(|(id, node_id)| { - (id, packages_to_resolved_id.get(&node_id).unwrap().clone()) + .map(|(nv, node_id)| { + ( + (*nv).clone(), + packages_to_resolved_id.get(&node_id).unwrap().clone(), + ) }) .collect(), packages_by_name: packages_by_name @@ -618,8 +629,16 @@ impl Graph { }) .collect(), packages, - package_reqs: self.package_reqs, - pending_unresolved_packages: self.pending_unresolved_packages, + package_reqs: self + .package_reqs + .into_iter() + .map(|(req, nv)| (req, (*nv).clone())) + .collect(), + pending_unresolved_packages: self + .pending_unresolved_packages + .into_iter() + .map(|nv| (*nv).clone()) + .collect(), }) } @@ -686,12 +705,12 @@ impl Graph { } #[derive(Default)] -struct DepEntryCache(HashMap>>); +struct DepEntryCache(HashMap, Arc>>); impl DepEntryCache { pub fn store( &mut self, - nv: NpmPackageNv, + nv: Arc, version_info: &NpmPackageVersionInfo, ) -> Result>, AnyError> { debug_assert!(!self.0.contains_key(&nv)); // we should not be re-inserting @@ -723,7 +742,8 @@ pub struct GraphDependencyResolver<'a> { graph: &'a mut Graph, api: &'a NpmRegistryApi, pending_unresolved_nodes: VecDeque>, - unresolved_optional_peers: HashMap>, + unresolved_optional_peers: + HashMap, Vec>, dep_entry_cache: DepEntryCache, } @@ -753,16 +773,16 @@ impl<'a> GraphDependencyResolver<'a> { let version_req = VersionReq::parse_from_specifier(&format!("{}", package_nv.version)) .unwrap(); - let (pkg_id, node_id) = self.resolve_node_from_info( + let (pkg_nv, node_id) = self.resolve_node_from_info( &package_nv.name, &version_req, package_info, None, )?; - self.graph.root_packages.insert(pkg_id.clone(), node_id); + self.graph.root_packages.insert(pkg_nv.clone(), node_id); self .pending_unresolved_nodes - .push_back(GraphPath::for_root(node_id, pkg_id)); + .push_back(GraphPath::for_root(node_id, pkg_nv)); Ok(()) } @@ -844,7 +864,7 @@ impl<'a> GraphDependencyResolver<'a> { version_req: &VersionReq, package_info: &NpmPackageInfo, parent_id: Option, - ) -> Result<(NpmPackageNv, NodeId), AnyError> { + ) -> Result<(Arc, NodeId), AnyError> { let version_and_info = resolve_best_package_version_and_info( version_req, package_info, @@ -856,21 +876,21 @@ impl<'a> GraphDependencyResolver<'a> { .iter(), )?; let resolved_id = ResolvedId { - nv: NpmPackageNv { + nv: Arc::new(NpmPackageNv { name: package_info.name.to_string(), version: version_and_info.version.clone(), - }, + }), peer_dependencies: Vec::new(), }; let (_, node_id) = self.graph.get_or_create_for_id(&resolved_id); - let pkg_id = resolved_id.nv; + let pkg_nv = resolved_id.nv; - let has_deps = if let Some(deps) = self.dep_entry_cache.get(&pkg_id) { + let has_deps = if let Some(deps) = self.dep_entry_cache.get(&pkg_nv) { !deps.is_empty() } else { let deps = self .dep_entry_cache - .store(pkg_id.clone(), version_and_info.info)?; + .store(pkg_nv.clone(), version_and_info.info)?; !deps.is_empty() }; @@ -888,10 +908,10 @@ impl<'a> GraphDependencyResolver<'a> { }, pkg_req_name, version_req.version_text(), - pkg_id.to_string(), + pkg_nv.to_string(), ); - Ok((pkg_id, node_id)) + Ok((pkg_nv, node_id)) } pub async fn resolve_pending(&mut self) -> Result<(), AnyError> { @@ -1099,11 +1119,7 @@ impl<'a> GraphDependencyResolver<'a> { if let Some(child_id) = find_matching_child( peer_dep, peer_package_info, - self - .graph - .root_packages - .iter() - .map(|(pkg_id, id)| (*id, pkg_id)), + self.graph.root_packages.iter().map(|(nv, id)| (*id, nv)), )? { let peer_parent = GraphPathNodeOrRoot::Root(root_pkg_id.clone()); self.set_new_peer_dep(&path, peer_parent, specifier, child_id); @@ -1179,7 +1195,7 @@ impl<'a> GraphDependencyResolver<'a> { // path from the node above the resolved dep to just above the peer dep path: &[&Arc], peer_dep: &ResolvedIdPeerDep, - peer_dep_nv: &NpmPackageNv, + peer_dep_nv: &Arc, peer_dep_id: Option, ) -> Option> { debug_assert!(!path.is_empty()); @@ -1372,7 +1388,7 @@ impl<'a> GraphDependencyResolver<'a> { fn find_matching_child<'a>( peer_dep: &NpmDependencyEntry, peer_package_info: &NpmPackageInfo, - children: impl Iterator, + children: impl Iterator)>, ) -> Result, AnyError> { for (child_id, pkg_id) in children { if pkg_id.name == peer_dep.name @@ -1403,7 +1419,7 @@ mod test { let mut ids = ResolvedNodeIds::default(); let node_id = NodeId(0); let resolved_id = ResolvedId { - nv: NpmPackageNv::from_str("package@1.1.1").unwrap(), + nv: Arc::new(NpmPackageNv::from_str("package@1.1.1").unwrap()), peer_dependencies: Vec::new(), }; ids.set(node_id, resolved_id.clone()); @@ -1412,7 +1428,7 @@ mod test { assert_eq!(ids.get_node_id(&resolved_id), Some(node_id)); let resolved_id_new = ResolvedId { - nv: NpmPackageNv::from_str("package@1.1.2").unwrap(), + nv: Arc::new(NpmPackageNv::from_str("package@1.1.2").unwrap()), peer_dependencies: Vec::new(), }; ids.set(node_id, resolved_id_new.clone()); From e42656a8e397accbe4dee9864ba51a639659cc0b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 14:47:08 -0500 Subject: [PATCH 08/15] Add test. --- cli/npm/resolution/graph.rs | 148 +++++++++++++++++++++++++++++++++++- 1 file changed, 145 insertions(+), 3 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 3438fc39ecbc1a..05a08daa81a752 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -986,7 +986,8 @@ impl<'a> GraphDependencyResolver<'a> { child_nv, ); if let Some(ancestor) = maybe_ancestor { - // it's circular, so mark it as such in the ancestor + // when the nv appears as an ancestor, use that node + // and mark this as circular self.add_linked_circular_descendant(&ancestor, child_path); } else { // mark the child as pending @@ -3853,7 +3854,7 @@ mod test { } #[tokio::test] - async fn resolve_dep_with_peer_deps_circular() { + async fn resolve_dep_with_peer_deps_circular_scenario_1() { // a -> b -> c -> d -> c where c has a peer dependency on b let api = TestNpmRegistryApiInner::default(); api.ensure_package_version("package-a", "1.0.0"); @@ -3863,8 +3864,8 @@ mod test { api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); api.add_dependency(("package-c", "1.0.0"), ("package-d", "1")); - api.add_peer_dependency(("package-c", "1.0.0"), ("package-b", "1")); api.add_dependency(("package-d", "1.0.0"), ("package-c", "1")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-b", "1")); let (packages, package_reqs) = run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; @@ -3930,6 +3931,147 @@ mod test { ); } + #[tokio::test] + async fn resolve_dep_with_peer_deps_circular_scenario_2() { + // a -> b -> c -> d -> c where c has a peer dependency on b + // -> e -> f -> d -> c where f has a peer dep on a + let api = TestNpmRegistryApiInner::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "1.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.ensure_package_version("package-d", "1.0.0"); + api.ensure_package_version("package-e", "1.0.0"); + api.ensure_package_version("package-f", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); + api.add_dependency(("package-c", "1.0.0"), ("package-d", "1")); + api.add_dependency(("package-c", "1.0.0"), ("package-e", "1")); + api.add_dependency(("package-d", "1.0.0"), ("package-c", "1")); + api.add_dependency(("package-e", "1.0.0"), ("package-f", "1")); + api.add_dependency(("package-f", "1.0.0"), ("package-d", "1")); + api.add_peer_dependency(("package-f", "1.0.0"), ("package-a", "1")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-b", "1")); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; + assert_eq!( + packages, + vec![ + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@1.0.0_package-a@1.0.0") + .unwrap(), + ),]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-b@1.0.0_package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized( + "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([ + ( + "package-b".to_string(), + NpmPackageId::from_serialized( + "package-b@1.0.0_package-a@1.0.0" + ) + .unwrap(), + ), + ( + "package-d".to_string(), + NpmPackageId::from_serialized( + "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + ), + ( + "package-e".to_string(), + NpmPackageId::from_serialized( + "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" + ) + .unwrap() + ) + ]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-c".to_string(), + NpmPackageId::from_serialized( + "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-f".to_string(), + NpmPackageId::from_serialized( + "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" + ) + .unwrap(), + )]), + dist: Default::default(), + }, + NpmResolutionPackage { + pkg_id: NpmPackageId::from_serialized( + "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" + ) + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-a".to_string(), + NpmPackageId::from_serialized( + "package-a@1.0.0" + ) + .unwrap(), + ), ( + "package-d".to_string(), + NpmPackageId::from_serialized( + "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" + ) + .unwrap(), + )]), + dist: Default::default(), + }, + ] + ); + assert_eq!( + package_reqs, + vec![("package-a@1.0.0".to_string(), "package-a@1.0.0".to_string())] + ); + } + async fn run_resolver_and_get_output( api: TestNpmRegistryApiInner, reqs: Vec<&str>, From 303c82f63cc4f676bddf6a633535e7ec1f73dc07 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 15:02:40 -0500 Subject: [PATCH 09/15] Better tests. --- cli/npm/resolution/graph.rs | 1545 ++++++++++++----------------------- 1 file changed, 526 insertions(+), 1019 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 05a08daa81a752..cdb67de68b10f2 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -1456,40 +1456,30 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ - ( - "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), - ), - ( - "package-c".to_string(), - NpmPackageId::from_serialized("package-c@0.1.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-b".to_string(), "package-b@2.0.0".to_string(),), + ("package-c".to_string(), "package-c@0.1.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-c@0.1.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@0.1.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@3.2.1").unwrap(), + "package-d@3.2.1".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-d@3.2.1").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@3.2.1".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -1513,23 +1503,21 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + "package-b@2.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -1556,36 +1544,26 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, dependencies: Default::default(), - dist: Default::default(), } ] ); @@ -1621,57 +1599,37 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-0@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + "package-a@1.0.0_package-peer@1.0.0".to_string(), ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), - ) + ("package-peer".to_string(), "package-peer@1.0.0".to_string(),) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, dependencies: Default::default(), - dist: Default::default(), } ] ); @@ -1703,67 +1661,45 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-1".to_string(), - NpmPackageId::from_serialized("package-1@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-1@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-1@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-1@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + "package-a@1.0.0_package-peer@1.0.0".to_string(), ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), - ) + ("package-peer".to_string(), "package-peer@1.0.0".to_string(),) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, dependencies: Default::default(), - dist: Default::default(), } ] ); @@ -1796,58 +1732,39 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + "package-b@2.0.0_package-peer@4.0.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + "package-c@3.0.0_package-peer@4.0.0".to_string(), ), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -1890,72 +1807,48 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-0@1.1.1").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.1.1".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-peer@4.0.0") - .unwrap(), + "package-a@1.0.0_package-peer@4.0.0".to_string(), ),]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + "package-b@2.0.0_package-peer@4.0.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), - ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-c@3.0.0_package-peer@4.0.0".to_string(), ), + ("package-peer".to_string(), "package-peer@4.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -1986,55 +1879,39 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.1.0" - ) - .unwrap(), + "package-b@2.0.0_package-peer@4.1.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.1.0" - ) - .unwrap(), + "package-c@3.0.0_package-peer@4.1.0".to_string(), ), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.1.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer@4.1.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.1.0").unwrap(), + "package-peer@4.1.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.1.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0_package-peer@4.1.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.1.0").unwrap(), + "package-peer@4.1.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@4.1.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@4.1.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -2071,32 +1948,23 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ - ( - "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), - ), - ( - "package-c".to_string(), - NpmPackageId::from_serialized("package-c@3.0.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-b".to_string(), "package-b@2.0.0".to_string(),), + ("package-c".to_string(), "package-c@3.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-c@3.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, ] ); @@ -2133,58 +2001,39 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + "package-b@2.0.0_package-peer@4.0.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + "package-c@3.0.0_package-peer@4.0.0".to_string(), ), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@4.0.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -2229,44 +2078,29 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), - ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), ), + ("package-peer".to_string(), "package-peer@1.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, ] ); @@ -2307,44 +2141,29 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@2.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + "package-peer@2.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + "package-a@1.0.0_package-peer@2.0.0".to_string(), ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), - ) + ("package-peer".to_string(), "package-peer@2.0.0".to_string(),) ]), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, ] ); @@ -2381,23 +2200,18 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, ] ); @@ -2442,62 +2256,42 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@2.0.0".to_string(), copy_index: 1, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + "package-peer@2.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([ - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-peer".to_string(), "package-peer@2.0.0".to_string(),), ( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + "package-a@1.0.0_package-peer@2.0.0".to_string(), ), ]), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::new(), + dependencies: Default::default(), }, ] ); @@ -2536,28 +2330,20 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@2.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@2.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-peer".to_string(), "package-peer@2.0.0".to_string(),), ( "package-peer2".to_string(), - NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + "package-peer@2.0.0".to_string(), ), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@2.0.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -2588,39 +2374,27 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-0@1.0.0_package-peer-a@2.0.0__package-peer-b@3.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0_package-peer-a@2.0.0__package-peer-b@3.0.0" + .to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer-a".to_string(), - NpmPackageId::from_serialized( - "package-peer-a@2.0.0_package-peer-b@3.0.0" - ) - .unwrap(), + "package-peer-a@2.0.0_package-peer-b@3.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-peer-a@2.0.0_package-peer-b@3.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-a@2.0.0_package-peer-b@3.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer-b".to_string(), - NpmPackageId::from_serialized("package-peer-b@3.0.0").unwrap(), + "package-peer-b@3.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer-b@3.0.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-b@3.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), }, ] ); @@ -2659,47 +2433,34 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-0@1.0.0_package-peer-a@2.0.0__package-peer-b@3.0.0_package-peer-b@3.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0_package-peer-a@2.0.0__package-peer-b@3.0.0_package-peer-b@3.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-peer-a".to_string(), - NpmPackageId::from_serialized( - "package-peer-a@2.0.0_package-peer-b@3.0.0" - ) - .unwrap(), + "package-peer-a@2.0.0_package-peer-b@3.0.0".to_string(), ), ( "package-peer-b".to_string(), - NpmPackageId::from_serialized("package-peer-b@3.0.0") - .unwrap(), + "package-peer-b@3.0.0".to_string(), ) ]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-peer-a@2.0.0_package-peer-b@3.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-a@2.0.0_package-peer-b@3.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer-b".to_string(), - NpmPackageId::from_serialized("package-peer-b@3.0.0") - .unwrap(), + "package-peer-b@3.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer-b@3.0.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-b@3.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), + }, ] ); @@ -2761,124 +2522,88 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-0@1.1.1") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.1.1".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1" - ) - .unwrap(), + "package-a@1.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1".to_string(), ),]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1_package-peer-c@6.2.0" - ) - .unwrap(), + "package-b@2.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1_package-peer-c@6.2.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1" - ) - .unwrap(), + "package-c@3.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1".to_string(), ), ( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@3.5.0").unwrap(), + "package-d@3.5.0".to_string(), ), ( "package-peer-a".to_string(), - NpmPackageId::from_serialized( - "package-peer-a@4.0.0_package-peer-b@5.4.1" - ) - .unwrap(), + "package-peer-a@4.0.0_package-peer-b@5.4.1".to_string(), ), ]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1_package-peer-c@6.2.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1_package-peer-c@6.2.0".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-peer-a".to_string(), - NpmPackageId::from_serialized("package-peer-a@4.0.0_package-peer-b@5.4.1") - .unwrap(), + "package-peer-a@4.0.0_package-peer-b@5.4.1".to_string(), ), ( "package-peer-c".to_string(), - NpmPackageId::from_serialized("package-peer-c@6.2.0") - .unwrap(), + "package-peer-c@6.2.0".to_string(), ) ]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@3.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@3.0.0_package-peer-a@4.0.0__package-peer-b@5.4.1".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer-a".to_string(), - NpmPackageId::from_serialized("package-peer-a@4.0.0_package-peer-b@5.4.1") - .unwrap(), + "package-peer-a@4.0.0_package-peer-b@5.4.1".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-d@3.5.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@3.5.0".to_string(), copy_index: 0, - dependencies: HashMap::from([]), - dist: Default::default(), + dependencies: BTreeMap::from([]), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-e@3.6.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-e@3.6.0".to_string(), copy_index: 0, - dependencies: HashMap::from([]), - dist: Default::default(), + dependencies: BTreeMap::from([]), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer-a@4.0.0_package-peer-b@5.4.1") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-a@4.0.0_package-peer-b@5.4.1".to_string(), copy_index: 0, - dist: Default::default(), - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer-b".to_string(), - NpmPackageId::from_serialized("package-peer-b@5.4.1") - .unwrap(), + "package-peer-b@5.4.1".to_string(), )]) }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer-b@5.4.1") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-b@5.4.1".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer-c@6.2.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer-c@6.2.0".to_string(), copy_index: 0, - dist: Default::default(), dependencies: Default::default(), }, ] @@ -2905,27 +2630,21 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0_package-a@1.0.0") - .unwrap(), + "package-b@2.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -2959,85 +2678,53 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-dep".to_string(), - NpmPackageId::from_serialized( - "package-dep@3.0.0_package-peer@4.0.0" - ) - .unwrap(), - ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-dep@3.0.0_package-peer@4.0.0".to_string(), ), + ("package-peer".to_string(), "package-peer@4.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-peer@5.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-peer@5.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-dep".to_string(), - NpmPackageId::from_serialized( - "package-dep@3.0.0_package-peer@5.0.0" - ) - .unwrap(), - ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@5.0.0").unwrap(), + "package-dep@3.0.0_package-peer@5.0.0".to_string(), ), + ("package-peer".to_string(), "package-peer@5.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-dep@3.0.0_package-peer@4.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-dep@3.0.0_package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@4.0.0").unwrap(), + "package-peer@4.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-dep@3.0.0_package-peer@5.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-dep@3.0.0_package-peer@5.0.0".to_string(), copy_index: 1, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@5.0.0").unwrap(), + "package-peer@5.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@4.0.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@4.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@5.0.0") - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@5.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), }, ] ); @@ -3077,57 +2764,39 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-b@1.0.0__package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-b@1.0.0__package-peer@1.0.0" + .to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0__package-peer@1.0.0" - ) - .unwrap(), + "package-c@1.0.0_package-b@1.0.0__package-peer@1.0.0".to_string(), ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), - ) + ("package-peer".to_string(), "package-peer@1.0.0".to_string(),) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0__package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-b@1.0.0__package-peer@1.0.0" + .to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([]), - dist: Default::default(), + dependencies: BTreeMap::from([]), }, ] ); @@ -3167,75 +2836,50 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.1.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.1.0".to_string(), copy_index: 1, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.1.0").unwrap(), + "package-peer@1.1.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.2.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.2.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.2.0").unwrap(), + "package-peer@1.2.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.1.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.1.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@1.0.0_package-peer@1.1.0" - ) - .unwrap(), + "package-c@1.0.0_package-peer@1.1.0".to_string(), ), - ( - "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.1.0").unwrap(), - ) + ("package-peer".to_string(), "package-peer@1.1.0".to_string(),) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-peer@1.1.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-peer@1.1.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-peer@1.1.0") - .unwrap(), + "package-a@1.0.0_package-peer@1.1.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.1.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.1.0".to_string(), copy_index: 0, - dependencies: HashMap::from([]), - dist: Default::default(), + dependencies: BTreeMap::from([]), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.2.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.2.0".to_string(), copy_index: 0, - dependencies: HashMap::from([]), - dist: Default::default(), + dependencies: BTreeMap::from([]), }, ] ); @@ -3277,65 +2921,44 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-d@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-d@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ - ( - "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-b".to_string(), "package-b@2.0.0".to_string(),), ( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-d@1.0.0") - .unwrap(), - ), - ( - "package-d".to_string(), - NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), - ), - ( - "package-e".to_string(), - NpmPackageId::from_serialized("package-e@1.0.0").unwrap(), + "package-c@1.0.0_package-d@1.0.0".to_string(), ), + ("package-d".to_string(), "package-d@1.0.0".to_string(),), + ("package-e".to_string(), "package-e@1.0.0".to_string(),), ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-d@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-d@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + "package-d@1.0.0".to_string(), ),]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::new(), - dist: Default::default(), + dependencies: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-e@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-e@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + "package-b@2.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -3358,12 +2981,11 @@ mod test { run_resolver_and_get_output(api, vec!["npm:package-a@1.0"]).await; assert_eq!( packages, - vec![NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + vec![TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, // in this case, we just ignore that the package did this dependencies: Default::default(), - dist: Default::default(), }] ); assert_eq!( @@ -3384,20 +3006,18 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@0.5.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@0.5.0".to_string(), copy_index: 0, dependencies: Default::default(), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@0.5.0").unwrap(), + "package-a@0.5.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -3420,27 +3040,21 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0_package-a@1.0.0") - .unwrap(), + "package-b@2.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), } ] ); @@ -3465,36 +3079,29 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@2.0.0_package-a@1.0.0") - .unwrap(), + "package-b@2.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@2.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@2.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), } ] ); @@ -3519,40 +3126,29 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-a@1.0.0") - .unwrap(), + "package-b@1.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-a@1.0.0") - .unwrap(), + "package-c@1.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + "package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -3577,36 +3173,29 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + "package-b@1.0.0".to_string(), ),]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") - .unwrap(), + "package-c@1.0.0_package-b@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-b@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + "package-b@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -3643,100 +3232,85 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + "package-a@1.0.0_package-0@1.0.0".to_string(), ), ( "package-1".to_string(), - NpmPackageId::from_serialized("package-1@1.0.0_package-0@1.0.0").unwrap(), + "package-1@1.0.0_package-0@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-1@1.0.0_package-0@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-1@1.0.0_package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + "package-a@1.0.0_package-0@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0") - .unwrap(), + "package-b@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-0".to_string(), - NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + "package-0@1.0.0".to_string(), ), ( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + "package-a@1.0.0_package-0@1.0.0".to_string(), ), ( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0") - .unwrap(), + "package-c@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), ) ]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-0".to_string(), - NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + "package-0@1.0.0".to_string(), ), ( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + "package-a@1.0.0_package-0@1.0.0".to_string(), ), ( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0") - .unwrap(), + "package-d@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), ) ]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-d@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@1.0.0_package-0@1.0.0_package-a@1.0.0__package-0@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-0".to_string(), - NpmPackageId::from_serialized("package-0@1.0.0").unwrap(), + "package-0@1.0.0".to_string(), ), ( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-0@1.0.0").unwrap(), + "package-a@1.0.0_package-0@1.0.0".to_string(), ) ]), - dist: Default::default(), + } ] ); @@ -3768,73 +3342,48 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-0@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-0@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-1".to_string(), - NpmPackageId::from_serialized( - "package-1@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + "package-1@1.0.0_package-peer@1.0.0".to_string(), ), ( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + "package-a@1.0.0_package-peer@1.0.0".to_string(), ) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-1@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-1@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized("package-a@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-a@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-a@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-peer@1.0.0") - .unwrap(), + "package-b@1.0.0_package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-peer@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-peer@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-peer".to_string(), - NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + "package-peer@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-peer@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-peer@1.0.0".to_string(), copy_index: 0, dependencies: Default::default(), - dist: Default::default(), } ] ); @@ -3872,56 +3421,40 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + "package-b@1.0.0".to_string(), ),]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") - .unwrap(), + "package-c@1.0.0_package-b@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-b@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ - ( - "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0").unwrap(), - ), + dependencies: BTreeMap::from([ + ("package-b".to_string(), "package-b@1.0.0".to_string(),), ( "package-d".to_string(), - NpmPackageId::from_serialized("package-d@1.0.0_package-b@1.0.0") - .unwrap(), + "package-d@1.0.0_package-b@1.0.0".to_string(), ) ]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-d@1.0.0_package-b@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@1.0.0_package-b@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized("package-c@1.0.0_package-b@1.0.0") - .unwrap(), + "package-c@1.0.0_package-b@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -3957,112 +3490,67 @@ mod test { assert_eq!( packages, vec![ - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized("package-a@1.0.0").unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-b".to_string(), - NpmPackageId::from_serialized("package-b@1.0.0_package-a@1.0.0") - .unwrap(), - ),]), - dist: Default::default(), + "package-b@1.0.0_package-a@1.0.0".to_string(), + )]), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-b@1.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([ + dependencies: BTreeMap::from([ ( "package-b".to_string(), - NpmPackageId::from_serialized( - "package-b@1.0.0_package-a@1.0.0" - ) - .unwrap(), + "package-b@1.0.0_package-a@1.0.0".to_string(), ), ( "package-d".to_string(), - NpmPackageId::from_serialized( - "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), ), ( "package-e".to_string(), - NpmPackageId::from_serialized( - "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" - ) - .unwrap() + "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0".to_string() ) ]), - dist: Default::default(), + }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-c".to_string(), - NpmPackageId::from_serialized( - "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + "package-c@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-e@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-f".to_string(), - NpmPackageId::from_serialized( - "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" - ) - .unwrap(), + "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, - NpmResolutionPackage { - pkg_id: NpmPackageId::from_serialized( - "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0" - ) - .unwrap(), + TestNpmResolutionPackage { + pkg_id: "package-f@1.0.0_package-a@1.0.0_package-b@1.0.0__package-a@1.0.0".to_string(), copy_index: 0, - dependencies: HashMap::from([( + dependencies: BTreeMap::from([( "package-a".to_string(), - NpmPackageId::from_serialized( - "package-a@1.0.0" - ) - .unwrap(), + "package-a@1.0.0".to_string(), ), ( "package-d".to_string(), - NpmPackageId::from_serialized( - "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0" - ) - .unwrap(), + "package-d@1.0.0_package-b@1.0.0__package-a@1.0.0_package-a@1.0.0".to_string(), )]), - dist: Default::default(), }, ] ); @@ -4072,10 +3560,17 @@ mod test { ); } + #[derive(Debug, PartialEq, Eq)] + struct TestNpmResolutionPackage { + pub pkg_id: String, + pub copy_index: usize, + pub dependencies: BTreeMap, + } + async fn run_resolver_and_get_output( api: TestNpmRegistryApiInner, reqs: Vec<&str>, - ) -> (Vec, Vec<(String, String)>) { + ) -> (Vec, Vec<(String, String)>) { let mut graph = Graph::default(); let api = NpmRegistryApi::new_for_test(api); let mut resolver = GraphDependencyResolver::new(&mut graph, &api); @@ -4125,6 +3620,18 @@ mod test { }) .collect::>(); package_reqs.sort_by(|a, b| a.0.to_string().cmp(&b.0.to_string())); + let packages = packages + .into_iter() + .map(|pkg| TestNpmResolutionPackage { + pkg_id: pkg.pkg_id.as_serialized(), + copy_index: pkg.copy_index, + dependencies: pkg + .dependencies + .into_iter() + .map(|(key, value)| (key, value.as_serialized())) + .collect(), + }) + .collect(); (packages, package_reqs) } From 854365abde49c3b69cc11207dc968f66948d52a5 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 15:33:04 -0500 Subject: [PATCH 10/15] Remove this --- cli/npm/resolution/graph.rs | 103 ++++++++++++++++++++++++++++++------ 1 file changed, 86 insertions(+), 17 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index cdb67de68b10f2..e80b57cc567318 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -1197,7 +1197,6 @@ impl<'a> GraphDependencyResolver<'a> { path: &[&Arc], peer_dep: &ResolvedIdPeerDep, peer_dep_nv: &Arc, - peer_dep_id: Option, ) -> Option> { debug_assert!(!path.is_empty()); @@ -1211,14 +1210,9 @@ impl<'a> GraphDependencyResolver<'a> { .unwrap() .clone(); - // todo: get rid of passing this in? - let is_current_ancestor_peer_dep = Some(old_node_id) == peer_dep_id; - if is_current_ancestor_peer_dep { - debug_assert_eq!(old_resolved_id.nv, *peer_dep_nv); + if old_resolved_id.nv == *peer_dep_nv { found_ancestor_node = Some((*graph_path_node).clone()); continue; - } else if old_resolved_id.nv == *peer_dep_nv { - continue; } let mut new_resolved_id = old_resolved_id.clone(); @@ -1248,7 +1242,7 @@ impl<'a> GraphDependencyResolver<'a> { graph_path_node.linked_circular_descendants.lock().clone(); for descendant in circular_descendants { let path = descendant.get_path_to_ancestor(new_node_id); - self.add_peer_dep_to_path(&path, peer_dep, peer_dep_nv, None); + self.add_peer_dep_to_path(&path, peer_dep, peer_dep_nv); descendant.change_id(new_node_id); // update the parent to point to this new node id @@ -1300,12 +1294,8 @@ impl<'a> GraphDependencyResolver<'a> { child_pkg_nv: peer_dep_nv.clone(), }; - let found_ancestor_node = self.add_peer_dep_to_path( - path, - &peer_dep, - &peer_dep_nv, - Some(peer_dep_id), - ); + let found_ancestor_node = + self.add_peer_dep_to_path(path, &peer_dep, &peer_dep_nv); // now set the peer dependency let bottom_node = path.first().unwrap(); @@ -1372,7 +1362,7 @@ impl<'a> GraphDependencyResolver<'a> { .nv .clone(), }; - self.add_peer_dep_to_path(&path, &peer_dep, &peer_dep_nv, None); + self.add_peer_dep_to_path(&path, &peer_dep, &peer_dep_nv); } let parent_node_id = path[0].node_id(); @@ -3403,7 +3393,7 @@ mod test { } #[tokio::test] - async fn resolve_dep_with_peer_deps_circular_scenario_1() { + async fn resolve_dep_with_peer_deps_circular_1() { // a -> b -> c -> d -> c where c has a peer dependency on b let api = TestNpmRegistryApiInner::default(); api.ensure_package_version("package-a", "1.0.0"); @@ -3465,7 +3455,7 @@ mod test { } #[tokio::test] - async fn resolve_dep_with_peer_deps_circular_scenario_2() { + async fn resolve_dep_with_peer_deps_circular_2() { // a -> b -> c -> d -> c where c has a peer dependency on b // -> e -> f -> d -> c where f has a peer dep on a let api = TestNpmRegistryApiInner::default(); @@ -3560,6 +3550,85 @@ mod test { ); } + #[tokio::test] + async fn resolve_dep_with_peer_deps_circular_3() { + // a -> b -> c -> d -> c (peer) + // -> e -> a (peer) + let api = TestNpmRegistryApiInner::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "1.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.ensure_package_version("package-d", "1.0.0"); + api.ensure_package_version("package-e", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "1")); + api.add_dependency(("package-b", "1.0.0"), ("package-c", "1")); + api.add_dependency(("package-c", "1.0.0"), ("package-d", "1")); + api.add_dependency(("package-d", "1.0.0"), ("package-e", "1")); + api.add_peer_dependency(("package-d", "1.0.0"), ("package-c", "1")); + api.add_peer_dependency(("package-e", "1.0.0"), ("package-a", "1")); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0.0"]).await; + assert_eq!( + packages, + vec![ + TestNpmResolutionPackage { + pkg_id: "package-a@1.0.0".to_string(), + copy_index: 0, + dependencies: BTreeMap::from([( + "package-b".to_string(), + "package-b@1.0.0_package-a@1.0.0".to_string(), + ),]), + }, + TestNpmResolutionPackage { + pkg_id: "package-b@1.0.0_package-a@1.0.0".to_string(), + copy_index: 0, + dependencies: BTreeMap::from([( + "package-c".to_string(), + "package-c@1.0.0_package-a@1.0.0".to_string(), + )]), + }, + TestNpmResolutionPackage { + pkg_id: "package-c@1.0.0_package-a@1.0.0".to_string(), + copy_index: 0, + dependencies: BTreeMap::from([( + "package-d".to_string(), + "package-d@1.0.0_package-c@1.0.0__package-a@1.0.0_package-a@1.0.0" + .to_string(), + )]), + }, + TestNpmResolutionPackage { + pkg_id: + "package-d@1.0.0_package-c@1.0.0__package-a@1.0.0_package-a@1.0.0" + .to_string(), + copy_index: 0, + dependencies: BTreeMap::from([ + ( + "package-c".to_string(), + "package-c@1.0.0_package-a@1.0.0".to_string(), + ), + ( + "package-e".to_string(), + "package-e@1.0.0_package-a@1.0.0".to_string() + ), + ]), + }, + TestNpmResolutionPackage { + pkg_id: "package-e@1.0.0_package-a@1.0.0".to_string(), + copy_index: 0, + dependencies: BTreeMap::from([( + "package-a".to_string(), + "package-a@1.0.0".to_string() + ),]), + }, + ] + ); + assert_eq!( + package_reqs, + vec![("package-a@1.0.0".to_string(), "package-a@1.0.0".to_string())] + ); + } + #[derive(Debug, PartialEq, Eq)] struct TestNpmResolutionPackage { pub pkg_id: String, From 122e6cf6e67539c54c283a3bc2c6cca98d42df11 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 15:57:51 -0500 Subject: [PATCH 11/15] Move out getting the ancestor node --- cli/npm/resolution/graph.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index e80b57cc567318..e6928cd4975deb 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -1197,10 +1197,9 @@ impl<'a> GraphDependencyResolver<'a> { path: &[&Arc], peer_dep: &ResolvedIdPeerDep, peer_dep_nv: &Arc, - ) -> Option> { + ) { debug_assert!(!path.is_empty()); - let mut found_ancestor_node = None; for graph_path_node in path.iter().rev() { let old_node_id = graph_path_node.node_id(); let old_resolved_id = self @@ -1211,7 +1210,6 @@ impl<'a> GraphDependencyResolver<'a> { .clone(); if old_resolved_id.nv == *peer_dep_nv { - found_ancestor_node = Some((*graph_path_node).clone()); continue; } @@ -1268,8 +1266,6 @@ impl<'a> GraphDependencyResolver<'a> { } } } - - found_ancestor_node } fn set_new_peer_dep( @@ -1294,8 +1290,13 @@ impl<'a> GraphDependencyResolver<'a> { child_pkg_nv: peer_dep_nv.clone(), }; - let found_ancestor_node = - self.add_peer_dep_to_path(path, &peer_dep, &peer_dep_nv); + let top_node = path.last().unwrap(); + let maybe_ancestor_node = if top_node.nv == peer_dep_nv { + Some(top_node) + } else { + None + }; + self.add_peer_dep_to_path(path, &peer_dep, &peer_dep_nv); // now set the peer dependency let bottom_node = path.first().unwrap(); @@ -1312,7 +1313,7 @@ impl<'a> GraphDependencyResolver<'a> { peer_dep_specifier.to_string(), peer_dep_nv, ); - if let Some(ancestor_node) = found_ancestor_node { + if let Some(ancestor_node) = maybe_ancestor_node { // it's circular, so link this in step with the ancestor node ancestor_node .linked_circular_descendants From c31fd356afe4733f2ed8c92f0caee3af1500326d Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 16:05:20 -0500 Subject: [PATCH 12/15] Do less work and prep for next refactor --- cli/npm/resolution/graph.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index e6928cd4975deb..0caadfa8963716 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -257,18 +257,18 @@ impl GraphPath { None } - /// Gets the bottom-up path to the ancestor, not including the current node. - pub fn get_path_to_ancestor( + /// Gets the bottom-up path to the ancestor not including the current or ancestor node. + pub fn get_path_to_ancestor_exclusive( &self, ancestor_node_id: NodeId, ) -> Vec<&Arc> { let mut path = Vec::new(); let mut maybe_next_node = self.previous_node.as_ref(); while let Some(GraphPathNodeOrRoot::Node(next_node)) = maybe_next_node { - path.push(next_node); if next_node.node_id() == ancestor_node_id { break; } + path.push(next_node); maybe_next_node = next_node.previous_node.as_ref(); } debug_assert!(maybe_next_node.is_some()); @@ -1239,7 +1239,7 @@ impl<'a> GraphDependencyResolver<'a> { let circular_descendants = graph_path_node.linked_circular_descendants.lock().clone(); for descendant in circular_descendants { - let path = descendant.get_path_to_ancestor(new_node_id); + let path = descendant.get_path_to_ancestor_exclusive(new_node_id); self.add_peer_dep_to_path(&path, peer_dep, peer_dep_nv); descendant.change_id(new_node_id); @@ -1291,10 +1291,11 @@ impl<'a> GraphDependencyResolver<'a> { }; let top_node = path.last().unwrap(); - let maybe_ancestor_node = if top_node.nv == peer_dep_nv { - Some(top_node) + let (maybe_circular_ancestor, path) = if top_node.nv == peer_dep_nv { + // it's circular, so exclude the top node + (Some(top_node), &path[0..path.len() - 1]) } else { - None + (None, path) }; self.add_peer_dep_to_path(path, &peer_dep, &peer_dep_nv); @@ -1313,7 +1314,7 @@ impl<'a> GraphDependencyResolver<'a> { peer_dep_specifier.to_string(), peer_dep_nv, ); - if let Some(ancestor_node) = maybe_ancestor_node { + if let Some(ancestor_node) = maybe_circular_ancestor { // it's circular, so link this in step with the ancestor node ancestor_node .linked_circular_descendants @@ -1341,7 +1342,7 @@ impl<'a> GraphDependencyResolver<'a> { descendant: Arc, ) { let ancestor_node_id = ancestor.node_id(); - let path = descendant.get_path_to_ancestor(ancestor_node_id); + let path = descendant.get_path_to_ancestor_exclusive(ancestor_node_id); let ancestor_resolved_id = self .graph From 571c8504200d740efbb66a39110f41f5b0eef0b9 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 16:50:12 -0500 Subject: [PATCH 13/15] Trying to add multiple peer deps to id at the same time --- cli/npm/resolution/graph.rs | 76 ++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 23 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 0caadfa8963716..00c6e3def09862 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -120,7 +120,7 @@ impl ResolvedId { let new_hash = peer_dep.current_state_hash(); for dep in &self.peer_dependencies { if new_hash == dep.current_state_hash() { - return false; + return false; // peer dep already set } } self.peer_dependencies.push(peer_dep); @@ -364,7 +364,7 @@ impl Graph { packages, created_package_ids, )?; - graph.set_child_of_parent_node(name, child_node_id, node_id); + graph.set_child_of_parent_node(node_id, name, child_node_id); } Ok(node_id) } @@ -531,9 +531,9 @@ impl Graph { fn set_child_of_parent_node( &mut self, + parent_id: NodeId, specifier: &str, child_id: NodeId, - parent_id: NodeId, ) { assert_ne!(child_id, parent_id); let parent = self.borrow_node_mut(parent_id); @@ -848,9 +848,9 @@ impl<'a> GraphDependencyResolver<'a> { self.add_linked_circular_descendant(&ancestor, new_path); } else { self.graph.set_child_of_parent_node( + parent_id, &entry.bare_specifier, child_id, - parent_id, ); self.pending_unresolved_nodes.push_back(new_path); } @@ -1191,12 +1191,11 @@ impl<'a> GraphDependencyResolver<'a> { } } - fn add_peer_dep_to_path( + fn add_peer_deps_to_path( &mut self, // path from the node above the resolved dep to just above the peer dep path: &[&Arc], - peer_dep: &ResolvedIdPeerDep, - peer_dep_nv: &Arc, + peer_deps: &[(&ResolvedIdPeerDep, Arc)], ) { debug_assert!(!path.is_empty()); @@ -1209,12 +1208,18 @@ impl<'a> GraphDependencyResolver<'a> { .unwrap() .clone(); - if old_resolved_id.nv == *peer_dep_nv { - continue; + let mut new_resolved_id = old_resolved_id; + let mut has_changed = false; + for (peer_dep, nv) in peer_deps { + if *nv == new_resolved_id.nv { + continue; + } + if new_resolved_id.push_peer_dep((*peer_dep).clone()) { + has_changed = true; + } } - let mut new_resolved_id = old_resolved_id.clone(); - if !new_resolved_id.push_peer_dep(peer_dep.clone()) { + if !has_changed { continue; // nothing to change } @@ -1227,28 +1232,28 @@ impl<'a> GraphDependencyResolver<'a> { // copy over the old children to this new one for (specifier, child_id) in &old_children { self.graph.set_child_of_parent_node( + new_node_id, specifier, *child_id, - new_node_id, ); } } - debug_assert_eq!(graph_path_node.node_id(), old_node_id); graph_path_node.change_id(new_node_id); + let circular_descendants = graph_path_node.linked_circular_descendants.lock().clone(); for descendant in circular_descendants { let path = descendant.get_path_to_ancestor_exclusive(new_node_id); - self.add_peer_dep_to_path(&path, peer_dep, peer_dep_nv); + self.add_peer_deps_to_path(&path, peer_deps); descendant.change_id(new_node_id); - // update the parent to point to this new node id - let parent_node_id = path[0].node_id(); + // update the bottom node to point to this new node id + let bottom_node_id = path[0].node_id(); self.graph.set_child_of_parent_node( + bottom_node_id, descendant.specifier(), descendant.node_id(), - parent_node_id, ); } @@ -1297,15 +1302,14 @@ impl<'a> GraphDependencyResolver<'a> { } else { (None, path) }; - self.add_peer_dep_to_path(path, &peer_dep, &peer_dep_nv); + self.add_peer_deps_to_path(path, &[(&peer_dep, peer_dep_nv.clone())]); // now set the peer dependency let bottom_node = path.first().unwrap(); - let parent_node_id = bottom_node.node_id(); self.graph.set_child_of_parent_node( + bottom_node.node_id(), peer_dep_specifier, peer_dep_id, - parent_node_id, ); // queue next step @@ -1364,14 +1368,40 @@ impl<'a> GraphDependencyResolver<'a> { .nv .clone(), }; - self.add_peer_dep_to_path(&path, &peer_dep, &peer_dep_nv); + self.add_peer_deps_to_path(&path, &[(&peer_dep, peer_dep_nv)]); } - let parent_node_id = path[0].node_id(); + // todo: for some reason this causes issues... + // let peer_deps = ancestor_resolved_id + // .peer_dependencies + // .iter() + // .map(|peer_dep| { + // ( + // peer_dep, + // match &peer_dep { + // ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { + // child_pkg_nv.clone() + // } + // ResolvedIdPeerDep::SnapshotNodeId(node_id) => self + // .graph + // .resolved_node_ids + // .get(*node_id) + // .unwrap() + // .nv + // .clone(), + // }, + // ) + // }) + // .collect::>(); + // if !peer_deps.is_empty() { + // self.add_peer_deps_to_path(&path, &peer_deps); + // } + + let bottom_node_id = path[0].node_id(); self.graph.set_child_of_parent_node( + bottom_node_id, descendant.specifier(), descendant.node_id(), - parent_node_id, ); ancestor.linked_circular_descendants.lock().push(descendant); From 0f4d6c54a555c5dad112bdacc06b9a12ea4027f2 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 17:41:50 -0500 Subject: [PATCH 14/15] Small updates. --- cli/npm/resolution/graph.rs | 396 ++++++++++++++++++------------------ 1 file changed, 198 insertions(+), 198 deletions(-) diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 00c6e3def09862..19e93884cf0cdd 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -130,6 +130,9 @@ impl ResolvedId { /// Mappings of node identifiers to resolved identifiers. Each node has exactly /// one resolved identifier. +/// +/// The mapping from resolved to node_ids is imprecise and will do a best attempt +/// at sharing nodes. #[derive(Default)] struct ResolvedNodeIds { node_to_resolved_id: HashMap, @@ -559,14 +562,14 @@ impl Graph { HashMap::with_capacity(self.nodes.len()); let mut traversed_node_ids = HashSet::with_capacity(self.nodes.len()); let mut pending = VecDeque::new(); - for root_id in self.root_packages.values() { - if traversed_node_ids.insert(*root_id) { - pending.push_back(*root_id); + for root_id in self.root_packages.values().copied() { + if traversed_node_ids.insert(root_id) { + pending + .push_back((root_id, packages_to_resolved_id.get(&root_id).unwrap())); } } - while let Some(node_id) = pending.pop_front() { + while let Some((node_id, resolved_id)) = pending.pop_front() { let node = self.nodes.get(&node_id).unwrap(); - let resolved_id = packages_to_resolved_id.get(&node_id).unwrap(); packages_by_name .entry(resolved_id.nv.name.clone()) @@ -579,34 +582,34 @@ impl Graph { .await? .unwrap_or_else(|| panic!("missing: {:?}", resolved_id.nv)) .dist; + + let mut dependencies = HashMap::with_capacity(node.children.len()); + let is_match = + resolved_id.as_serialized() == "@parcel/types@2.8.3_@parcel+core@2.8.3"; + if is_match { + eprintln!("{:?}: {}", node_id, resolved_id.as_serialized()); + } + for (specifier, child_id) in &node.children { + let child_id = *child_id; + let resolved_id = packages_to_resolved_id.get(&child_id).unwrap(); + if traversed_node_ids.insert(child_id) { + pending.push_back((child_id, resolved_id)); + } + dependencies.insert(specifier.clone(), (*resolved_id).clone()); + if is_match { + eprintln!(" {:?}: {}", child_id, resolved_id.as_serialized()); + } + } + packages.insert( (*resolved_id).clone(), NpmResolutionPackage { copy_index: copy_index_resolver.resolve(resolved_id), pkg_id: (*resolved_id).clone(), dist, - dependencies: node - .children - .iter() - .map(|(key, value)| { - ( - key.clone(), - packages_to_resolved_id - .get(value) - .unwrap_or_else(|| { - panic!("{node_id:?} -- missing child: {value:?}") - }) - .clone(), - ) - }) - .collect(), + dependencies, }, ); - for child_id in node.children.values() { - if traversed_node_ids.insert(*child_id) { - pending.push_back(*child_id); - } - } } Ok(NpmResolutionSnapshot { @@ -915,154 +918,152 @@ impl<'a> GraphDependencyResolver<'a> { } pub async fn resolve_pending(&mut self) -> Result<(), AnyError> { - while !self.pending_unresolved_nodes.is_empty() { - // now go down through the dependencies by tree depth - while let Some(parent_path) = self.pending_unresolved_nodes.pop_front() { - let (parent_nv, child_deps) = { - let node_id = parent_path.node_id(); - if self.graph.nodes.get(&node_id).unwrap().no_peers { - // We can skip as there's no reason to analyze this graph segment further. - continue; - } + // go down through the dependencies by tree depth + while let Some(parent_path) = self.pending_unresolved_nodes.pop_front() { + let (parent_nv, child_deps) = { + let node_id = parent_path.node_id(); + if self.graph.nodes.get(&node_id).unwrap().no_peers { + // We can skip as there's no reason to analyze this graph segment further. + continue; + } - let pkg_nv = self - .graph - .resolved_node_ids - .get(node_id) - .unwrap() - .nv - .clone(); - let deps = if let Some(deps) = self.dep_entry_cache.get(&pkg_nv) { - deps.clone() - } else { - // the api should have this in the cache at this point, so no need to parallelize - match self.api.package_version_info(&pkg_nv).await? { - Some(version_info) => { - self.dep_entry_cache.store(pkg_nv.clone(), &version_info)? - } - None => { - bail!("Could not find version information for {}", pkg_nv) - } + let pkg_nv = self + .graph + .resolved_node_ids + .get(node_id) + .unwrap() + .nv + .clone(); + let deps = if let Some(deps) = self.dep_entry_cache.get(&pkg_nv) { + deps.clone() + } else { + // the api should have this in the cache at this point, so no need to parallelize + match self.api.package_version_info(&pkg_nv).await? { + Some(version_info) => { + self.dep_entry_cache.store(pkg_nv.clone(), &version_info)? } - }; - - (pkg_nv, deps) + None => { + bail!("Could not find version information for {}", pkg_nv) + } + } }; - // cache all the dependencies' registry infos in parallel if should - self - .api - .cache_in_parallel({ - child_deps.iter().map(|dep| dep.name.clone()).collect() - }) - .await?; - - // resolve the dependencies - let mut found_peer = false; - - for dep in child_deps.iter() { - let package_info = self.api.package_info(&dep.name).await?; - - match dep.kind { - NpmDependencyEntryKind::Dep => { - let parent_id = parent_path.node_id(); - let node = self.graph.nodes.get(&parent_id).unwrap(); - let child_id = match node.children.get(&dep.bare_specifier) { - Some(child_id) => { - // this dependency was previously analyzed by another path - // so we don't attempt to resolve the version again - let child_id = *child_id; - let child_nv = self - .graph - .resolved_node_ids - .get(child_id) - .unwrap() - .nv - .clone(); - let maybe_ancestor = parent_path.find_ancestor(&child_nv); - let child_path = parent_path.with_id( - child_id, - dep.bare_specifier.clone(), - child_nv, - ); - if let Some(ancestor) = maybe_ancestor { - // when the nv appears as an ancestor, use that node - // and mark this as circular - self.add_linked_circular_descendant(&ancestor, child_path); - } else { - // mark the child as pending - self.pending_unresolved_nodes.push_back(child_path); - } - child_id - } - None => { - self.analyze_dependency(dep, &package_info, &parent_path)? - } - }; + (pkg_nv, deps) + }; - if !found_peer { - found_peer = !self.graph.borrow_node_mut(child_id).no_peers; + // cache all the dependencies' registry infos in parallel if should + self + .api + .cache_in_parallel({ + child_deps.iter().map(|dep| dep.name.clone()).collect() + }) + .await?; + + // resolve the dependencies + let mut found_peer = false; + + for dep in child_deps.iter() { + let package_info = self.api.package_info(&dep.name).await?; + + match dep.kind { + NpmDependencyEntryKind::Dep => { + let parent_id = parent_path.node_id(); + let node = self.graph.nodes.get(&parent_id).unwrap(); + let child_id = match node.children.get(&dep.bare_specifier) { + Some(child_id) => { + // this dependency was previously analyzed by another path + // so we don't attempt to resolve the version again + let child_id = *child_id; + let child_nv = self + .graph + .resolved_node_ids + .get(child_id) + .unwrap() + .nv + .clone(); + let maybe_ancestor = parent_path.find_ancestor(&child_nv); + let child_path = parent_path.with_id( + child_id, + dep.bare_specifier.clone(), + child_nv, + ); + if let Some(ancestor) = maybe_ancestor { + // when the nv appears as an ancestor, use that node + // and mark this as circular + self.add_linked_circular_descendant(&ancestor, child_path); + } else { + // mark the child as pending + self.pending_unresolved_nodes.push_back(child_path); + } + child_id + } + None => { + self.analyze_dependency(dep, &package_info, &parent_path)? } + }; + + if !found_peer { + found_peer = !self.graph.borrow_node_mut(child_id).no_peers; } - NpmDependencyEntryKind::Peer - | NpmDependencyEntryKind::OptionalPeer => { - found_peer = true; - // we need to re-evaluate peer dependencies every time and can't - // skip over them because they might be evaluated differently based - // on the current path - let maybe_new_id = self.resolve_peer_dep( - &dep.bare_specifier, - dep, - &package_info, - &parent_path, - )?; - - // For optional dependencies, we want to resolve them if any future - // same parent version resolves them. So when not resolved, store them to be - // potentially resolved later. - // - // Note: This is not a good solution, but will probably work ok in most - // scenarios. We can work on improving this in the future. We probably - // want to resolve future optional peers to the same dependency for example. - if dep.kind == NpmDependencyEntryKind::OptionalPeer { - match maybe_new_id { - Some(new_id) => { - if let Some(unresolved_optional_peers) = - self.unresolved_optional_peers.remove(&parent_nv) - { - for optional_peer in unresolved_optional_peers { - let peer_parent = GraphPathNodeOrRoot::Node( - optional_peer.graph_path.clone(), - ); - self.set_new_peer_dep( - &[&optional_peer.graph_path], - peer_parent, - &optional_peer.specifier, - new_id, - ); - } + } + NpmDependencyEntryKind::Peer + | NpmDependencyEntryKind::OptionalPeer => { + found_peer = true; + // we need to re-evaluate peer dependencies every time and can't + // skip over them because they might be evaluated differently based + // on the current path + let maybe_new_id = self.resolve_peer_dep( + &dep.bare_specifier, + dep, + &package_info, + &parent_path, + )?; + + // For optional dependencies, we want to resolve them if any future + // same parent version resolves them. So when not resolved, store them to be + // potentially resolved later. + // + // Note: This is not a good solution, but will probably work ok in most + // scenarios. We can work on improving this in the future. We probably + // want to resolve future optional peers to the same dependency for example. + if dep.kind == NpmDependencyEntryKind::OptionalPeer { + match maybe_new_id { + Some(new_id) => { + if let Some(unresolved_optional_peers) = + self.unresolved_optional_peers.remove(&parent_nv) + { + for optional_peer in unresolved_optional_peers { + let peer_parent = GraphPathNodeOrRoot::Node( + optional_peer.graph_path.clone(), + ); + self.set_new_peer_dep( + &[&optional_peer.graph_path], + peer_parent, + &optional_peer.specifier, + new_id, + ); } } - None => { - // store this for later if it's resolved for this version - self - .unresolved_optional_peers - .entry(parent_nv.clone()) - .or_default() - .push(UnresolvedOptionalPeer { - specifier: dep.bare_specifier.clone(), - graph_path: parent_path.clone(), - }); - } + } + None => { + // store this for later if it's resolved for this version + self + .unresolved_optional_peers + .entry(parent_nv.clone()) + .or_default() + .push(UnresolvedOptionalPeer { + specifier: dep.bare_specifier.clone(), + graph_path: parent_path.clone(), + }); } } } } } + } - if !found_peer { - self.graph.borrow_node_mut(parent_path.node_id()).no_peers = true; - } + if !found_peer { + self.graph.borrow_node_mut(parent_path.node_id()).no_peers = true; } } Ok(()) @@ -1355,48 +1356,47 @@ impl<'a> GraphDependencyResolver<'a> { .unwrap() .clone(); - for peer_dep in ancestor_resolved_id.peer_dependencies { - let peer_dep_nv = match &peer_dep { - ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { - child_pkg_nv.clone() - } - ResolvedIdPeerDep::SnapshotNodeId(node_id) => self - .graph - .resolved_node_ids - .get(*node_id) - .unwrap() - .nv - .clone(), - }; - self.add_peer_deps_to_path(&path, &[(&peer_dep, peer_dep_nv)]); - } - - // todo: for some reason this causes issues... - // let peer_deps = ancestor_resolved_id - // .peer_dependencies - // .iter() - // .map(|peer_dep| { - // ( - // peer_dep, - // match &peer_dep { - // ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { - // child_pkg_nv.clone() - // } - // ResolvedIdPeerDep::SnapshotNodeId(node_id) => self - // .graph - // .resolved_node_ids - // .get(*node_id) - // .unwrap() - // .nv - // .clone(), - // }, - // ) - // }) - // .collect::>(); - // if !peer_deps.is_empty() { - // self.add_peer_deps_to_path(&path, &peer_deps); + // for peer_dep in ancestor_resolved_id.peer_dependencies { + // let peer_dep_nv = match &peer_dep { + // ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { + // child_pkg_nv.clone() + // } + // ResolvedIdPeerDep::SnapshotNodeId(node_id) => self + // .graph + // .resolved_node_ids + // .get(*node_id) + // .unwrap() + // .nv + // .clone(), + // }; + // self.add_peer_deps_to_path(&path, &[(&peer_dep, peer_dep_nv)]); // } + let peer_deps = ancestor_resolved_id + .peer_dependencies + .iter() + .map(|peer_dep| { + ( + peer_dep, + match &peer_dep { + ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { + child_pkg_nv.clone() + } + ResolvedIdPeerDep::SnapshotNodeId(node_id) => self + .graph + .resolved_node_ids + .get(*node_id) + .unwrap() + .nv + .clone(), + }, + ) + }) + .collect::>(); + if !peer_deps.is_empty() { + self.add_peer_deps_to_path(&path, &peer_deps); + } + let bottom_node_id = path[0].node_id(); self.graph.set_child_of_parent_node( bottom_node_id, From d1c487faeb70ce15a7014389baf837fd44159e66 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 7 Mar 2023 18:44:18 -0500 Subject: [PATCH 15/15] Cleanup. --- cli/npm/cache.rs | 8 ++++- cli/npm/resolution/graph.rs | 71 +++++++++++++++---------------------- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/cli/npm/cache.rs b/cli/npm/cache.rs index 1bc2f8487233c3..81fb76772acb8c 100644 --- a/cli/npm/cache.rs +++ b/cli/npm/cache.rs @@ -15,6 +15,7 @@ use deno_core::parking_lot::Mutex; use deno_core::url::Url; use deno_graph::npm::NpmPackageNv; use deno_graph::semver::Version; +use once_cell::sync::Lazy; use crate::args::CacheSetting; use crate::cache::DenoDir; @@ -27,10 +28,15 @@ use crate::util::progress_bar::ProgressBar; use super::registry::NpmPackageVersionDistInfo; use super::tarball::verify_and_extract_tarball; +static SHOULD_SYNC_DOWNLOAD: Lazy = + Lazy::new(|| std::env::var("DENO_UNSTABLE_NPM_SYNC_DOWNLOAD").is_ok()); + /// For some of the tests, we want downloading of packages /// to be deterministic so that the output is always the same pub fn should_sync_download() -> bool { - std::env::var("DENO_UNSTABLE_NPM_SYNC_DOWNLOAD").is_ok() + // this gets called a lot when doing npm resolution and was taking + // a significant amount of time, so cache it in a lazy + *SHOULD_SYNC_DOWNLOAD } const NPM_PACKAGE_SYNC_LOCK_FILENAME: &str = ".deno_sync_lock"; diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index 19e93884cf0cdd..65754f3bf264db 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -33,6 +33,9 @@ use super::snapshot::NpmResolutionSnapshot; use super::NpmPackageId; use super::NpmResolutionPackage; +// todo(dsherret): for perf we should use an arena/bump allocator for +// creating the nodes and paths since this is done in a phase + /// A unique identifier to a node in the graph. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)] struct NodeId(u32); @@ -547,7 +550,7 @@ impl Graph { self, api: &NpmRegistryApi, ) -> Result { - let packages_to_resolved_id = self + let packages_to_pkg_ids = self .nodes .keys() .map(|node_id| (*node_id, self.get_npm_pkg_id(*node_id))) @@ -560,52 +563,52 @@ impl Graph { let mut packages = HashMap::with_capacity(self.nodes.len()); let mut packages_by_name: HashMap> = HashMap::with_capacity(self.nodes.len()); - let mut traversed_node_ids = HashSet::with_capacity(self.nodes.len()); + + // todo(dsherret): there is a lurking bug within the peer dependencies code. + // You can see it by using `NodeIds` instead of `NpmPackageIds` on this travered_ids + // hashset, which will cause the bottom of the "tree" nodes to be populated in + // the result instead of the top of the "tree". I think there's maybe one small + // thing that's not being updated properly. + let mut traversed_ids = HashSet::with_capacity(self.nodes.len()); let mut pending = VecDeque::new(); + for root_id in self.root_packages.values().copied() { - if traversed_node_ids.insert(root_id) { - pending - .push_back((root_id, packages_to_resolved_id.get(&root_id).unwrap())); + let pkg_id = packages_to_pkg_ids.get(&root_id).unwrap(); + if traversed_ids.insert(pkg_id.clone()) { + pending.push_back((root_id, pkg_id)); } } - while let Some((node_id, resolved_id)) = pending.pop_front() { + + while let Some((node_id, pkg_id)) = pending.pop_front() { let node = self.nodes.get(&node_id).unwrap(); packages_by_name - .entry(resolved_id.nv.name.clone()) + .entry(pkg_id.nv.name.clone()) .or_default() - .push(resolved_id.clone()); + .push(pkg_id.clone()); // todo(dsherret): grab this from the dep entry cache, which should have it let dist = api - .package_version_info(&resolved_id.nv) + .package_version_info(&pkg_id.nv) .await? - .unwrap_or_else(|| panic!("missing: {:?}", resolved_id.nv)) + .unwrap_or_else(|| panic!("missing: {:?}", pkg_id.nv)) .dist; let mut dependencies = HashMap::with_capacity(node.children.len()); - let is_match = - resolved_id.as_serialized() == "@parcel/types@2.8.3_@parcel+core@2.8.3"; - if is_match { - eprintln!("{:?}: {}", node_id, resolved_id.as_serialized()); - } for (specifier, child_id) in &node.children { let child_id = *child_id; - let resolved_id = packages_to_resolved_id.get(&child_id).unwrap(); - if traversed_node_ids.insert(child_id) { - pending.push_back((child_id, resolved_id)); - } - dependencies.insert(specifier.clone(), (*resolved_id).clone()); - if is_match { - eprintln!(" {:?}: {}", child_id, resolved_id.as_serialized()); + let child_pkg_id = packages_to_pkg_ids.get(&child_id).unwrap(); + if traversed_ids.insert(child_pkg_id.clone()) { + pending.push_back((child_id, child_pkg_id)); } + dependencies.insert(specifier.clone(), (*child_pkg_id).clone()); } packages.insert( - (*resolved_id).clone(), + (*pkg_id).clone(), NpmResolutionPackage { - copy_index: copy_index_resolver.resolve(resolved_id), - pkg_id: (*resolved_id).clone(), + copy_index: copy_index_resolver.resolve(pkg_id), + pkg_id: (*pkg_id).clone(), dist, dependencies, }, @@ -619,7 +622,7 @@ impl Graph { .map(|(nv, node_id)| { ( (*nv).clone(), - packages_to_resolved_id.get(&node_id).unwrap().clone(), + packages_to_pkg_ids.get(&node_id).unwrap().clone(), ) }) .collect(), @@ -1356,22 +1359,6 @@ impl<'a> GraphDependencyResolver<'a> { .unwrap() .clone(); - // for peer_dep in ancestor_resolved_id.peer_dependencies { - // let peer_dep_nv = match &peer_dep { - // ResolvedIdPeerDep::ParentReference { child_pkg_nv, .. } => { - // child_pkg_nv.clone() - // } - // ResolvedIdPeerDep::SnapshotNodeId(node_id) => self - // .graph - // .resolved_node_ids - // .get(*node_id) - // .unwrap() - // .nv - // .clone(), - // }; - // self.add_peer_deps_to_path(&path, &[(&peer_dep, peer_dep_nv)]); - // } - let peer_deps = ancestor_resolved_id .peer_dependencies .iter()