fix(resolver): fix issue with aliases not being handled properly

Fixes: #287
orogene · Sep 29, 2023 · 85d3171 · 85d3171
1 parent c024892
commit 85d3171
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 19 deletions.
diff --git a/crates/node-maintainer/src/graph.rs b/crates/node-maintainer/src/graph.rs
@@ -104,7 +104,7 @@ impl Node {
     }
 }
 
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
 pub enum DepType {
     Prod,
     Dev,

diff --git a/crates/node-maintainer/src/resolver.rs b/crates/node-maintainer/src/resolver.rs
@@ -116,7 +116,7 @@ impl<'a> Resolver<'a> {
                     let dep = NodeDependency {
                         name: name.clone(),
                         spec,
-                        dep_type: dep_type.clone(),
+                        dep_type,
                         node_idx,
                     };
 
@@ -156,10 +156,8 @@ impl<'a> Resolver<'a> {
 
                                 let child_idx = Self::place_child(
                                     &mut self.graph,
-                                    node_idx,
+                                    &dep,
                                     package,
-                                    &dep.spec,
-                                    dep_type,
                                     lockfile_node.into(),
                                     Some(target_path),
                                 )?;
@@ -229,10 +227,8 @@ impl<'a> Resolver<'a> {
 
                             let child_idx = Self::place_child(
                                 &mut self.graph,
-                                dep.node_idx,
+                                &dep,
                                 package.clone(),
-                                &dep.spec,
-                                dep.dep_type,
                                 manifest.clone(),
                                 None,
                             )?;
@@ -289,7 +285,7 @@ impl<'a> Resolver<'a> {
                 let edge_idx = graph.inner.add_edge(
                     dep.node_idx,
                     satisfier_idx,
-                    Edge::new(dep.spec.clone(), dep.dep_type.clone()),
+                    Edge::new(dep.spec.clone(), dep.dep_type),
                 );
                 graph[dep.node_idx]
                     .dependencies
@@ -346,14 +342,15 @@ impl<'a> Resolver<'a> {
 
     fn place_child(
         graph: &mut Graph,
-        dependent_idx: NodeIndex,
+        dep: &NodeDependency,
         package: Package,
-        requested: &PackageSpec,
-        dep_type: DepType,
         corgi: CorgiManifest,
         target_path: Option<Vec<UniCase<String>>>,
     ) -> Result<NodeIndex, NodeMaintainerError> {
-        let child_name = UniCase::new(package.name().to_string());
+        let child_name = &dep.name;
+        let requested = &dep.spec;
+        let dep_type = dep.dep_type;
+        let dependent_idx = dep.node_idx;
         let child_node = Node::new(package, corgi, false)?;
         let child_idx = graph.inner.add_node(child_node);
         graph[child_idx].root = graph.root;
@@ -376,7 +373,7 @@ impl<'a> Resolver<'a> {
         let mut target_idx = dependent_idx;
         let mut parent_idx = Some(dependent_idx);
         'outer: while let Some(curr_target_idx) = parent_idx {
-            if let Some(resolved) = graph.resolve_dep(curr_target_idx, &child_name) {
+            if let Some(resolved) = graph.resolve_dep(curr_target_idx, child_name) {
                 for edge_ref in graph.inner.edges_directed(resolved, Direction::Incoming) {
                     let (from, _) = graph
                         .inner
@@ -390,7 +387,7 @@ impl<'a> Resolver<'a> {
                 }
             }
 
-            if let Some((req, _)) = graph[curr_target_idx].dependency_reqs.get(&child_name) {
+            if let Some((req, _)) = graph[curr_target_idx].dependency_reqs.get(child_name) {
                 if !graph[child_idx].package.resolved().satisfies(req)? {
                     break 'outer;
                 }
@@ -401,7 +398,8 @@ impl<'a> Resolver<'a> {
             parent_idx = graph[curr_target_idx].parent;
 
             // Try and place it where the lockfile asks us to, but only if it
-            // makes sense for us to do so.
+            // makes sense for us to do so. If something's already there for
+            // some reason, we'll ignore placing this.
             if let Some(locked) = &target_path {
                 if locked == &graph.node_path(target_idx) {
                     break 'outer;
@@ -424,12 +422,14 @@ impl<'a> Resolver<'a> {
         {
             // Finally, we add the backlink from the parent node to the child.
             let node = &mut graph[target_idx];
-            if let Some(old) = node.children.insert(child_name, child_idx) {
+            if let Some(old) = node.children.insert(child_name.clone(), child_idx) {
                 tracing::error!(
-                    "clobbered {} with {} (requested: {}). This is a bug with the orogene resolver. Please report it.",
+                    "clobbered {} at {} with {} (requested: {} by {}). This is a bug with the orogene resolver. Please report it.",
                     graph[old].package.resolved(),
+                    graph.node_path(old).iter().map(|x| x.to_string()).collect::<Vec<_>>().join("/node_modules/"),
                     graph[child_idx].package.resolved(),
-                    requested
+                    requested,
+                    graph.node_path(dependent_idx).iter().map(|x| x.to_string()).collect::<Vec<_>>().join("/node_modules/")
                 );
             }
         }