fix(workspace): do not resolve to self for npm pkg depending on matching req (#24591)

Closes #24584

cli/args/package_json.rs

@@ -8,17 +8,26 @@ use deno_config::workspace::Workspace;
 use deno_semver::package::PackageReq;
 
 #[derive(Debug)]
-pub struct InstallNpmWorkspacePkg {
+pub struct InstallNpmRemotePkg {
   pub alias: String,
-  pub pkg_dir: PathBuf,
+  // todo(24419): use this when setting up the node_modules dir
+  #[allow(dead_code)]
+  pub base_dir: PathBuf,
+  pub req: PackageReq,
+}
+
+#[derive(Debug)]
+pub struct InstallNpmWorkspacePkg {
+  pub alias: String,
+  // todo(24419): use this when setting up the node_modules dir
+  #[allow(dead_code)]
+  pub base_dir: PathBuf,
+  pub target_dir: PathBuf,
 }
 
-// todo(#24419): this is not correct, but it's good enough for now.
-// We need deno_npm to be able to understand workspace packages and
-// then have a way to properly lay them out on the file system
 #[derive(Debug, Default)]
 pub struct PackageJsonInstallDepsProvider {
-  remote_pkg_reqs: Vec<PackageReq>,
+  remote_pkgs: Vec<InstallNpmRemotePkg>,
   workspace_pkgs: Vec<InstallNpmWorkspacePkg>,
 }
 
@@ -29,27 +38,35 @@ impl PackageJsonInstallDepsProvider {
 
   pub fn from_workspace(workspace: &Arc<Workspace>) -> Self {
     let mut workspace_pkgs = Vec::new();
-    let mut remote_pkg_reqs = Vec::new();
+    let mut remote_pkgs = Vec::new();
     let workspace_npm_pkgs = workspace.npm_packages();
     for pkg_json in workspace.package_jsons() {
       let deps = pkg_json.resolve_local_package_json_deps();
-      let mut pkg_reqs = Vec::with_capacity(deps.len());
+      let mut pkg_pkgs = Vec::with_capacity(deps.len());
       for (alias, dep) in deps {
         let Ok(dep) = dep else {
           continue;
         };
         match dep {
           PackageJsonDepValue::Req(pkg_req) => {
-            if let Some(pkg) = workspace_npm_pkgs
-              .iter()
-              .find(|pkg| pkg.matches_req(&pkg_req))
-            {
+            let workspace_pkg = workspace_npm_pkgs.iter().find(|pkg| {
+              pkg.matches_req(&pkg_req)
+              // do not resolve to the current package
+              && pkg.pkg_json.path != pkg_json.path
+            });
+
+            if let Some(pkg) = workspace_pkg {
               workspace_pkgs.push(InstallNpmWorkspacePkg {
                 alias,
-                pkg_dir: pkg.pkg_json.dir_path().to_path_buf(),
+                base_dir: pkg_json.dir_path().to_path_buf(),
+                target_dir: pkg.pkg_json.dir_path().to_path_buf(),
               });
             } else {
-              pkg_reqs.push(pkg_req)
+              pkg_pkgs.push(InstallNpmRemotePkg {
+                alias,
+                base_dir: pkg_json.dir_path().to_path_buf(),
+                req: pkg_req,
+              });
             }
           }
           PackageJsonDepValue::Workspace(version_req) => {
@@ -58,27 +75,28 @@ impl PackageJsonInstallDepsProvider {
             }) {
               workspace_pkgs.push(InstallNpmWorkspacePkg {
                 alias,
-                pkg_dir: pkg.pkg_json.dir_path().to_path_buf(),
+                base_dir: pkg_json.dir_path().to_path_buf(),
+                target_dir: pkg.pkg_json.dir_path().to_path_buf(),
               });
             }
           }
         }
       }
       // sort within each package
-      pkg_reqs.sort();
+      pkg_pkgs.sort_by(|a, b| a.alias.cmp(&b.alias));
 
-      remote_pkg_reqs.extend(pkg_reqs);
+      remote_pkgs.extend(pkg_pkgs);
     }
-    remote_pkg_reqs.shrink_to_fit();
+    remote_pkgs.shrink_to_fit();
     workspace_pkgs.shrink_to_fit();
     Self {
-      remote_pkg_reqs,
+      remote_pkgs,
       workspace_pkgs,
     }
   }
 
-  pub fn remote_pkg_reqs(&self) -> &Vec<PackageReq> {
-    &self.remote_pkg_reqs
+  pub fn remote_pkgs(&self) -> &Vec<InstallNpmRemotePkg> {
+    &self.remote_pkgs
   }
 
   pub fn workspace_pkgs(&self) -> &Vec<InstallNpmWorkspacePkg> {

cli/npm/managed/mod.rs

@@ -475,24 +475,30 @@ impl ManagedCliNpmResolver {
     if !self.top_level_install_flag.raise() {
       return Ok(false); // already did this
     }
-    let reqs = self.package_json_deps_provider.remote_pkg_reqs();
-    if reqs.is_empty() {
+    let pkg_json_remote_pkgs = self.package_json_deps_provider.remote_pkgs();
+    if pkg_json_remote_pkgs.is_empty() {
       return Ok(false);
     }
 
     // check if something needs resolving before bothering to load all
     // the package information (which is slow)
-    if reqs
-      .iter()
-      .all(|req| self.resolution.resolve_pkg_id_from_pkg_req(req).is_ok())
-    {
+    if pkg_json_remote_pkgs.iter().all(|pkg| {
+      self
+        .resolution
+        .resolve_pkg_id_from_pkg_req(&pkg.req)
+        .is_ok()
+    }) {
       log::debug!(
         "All package.json deps resolvable. Skipping top level install."
       );
       return Ok(false); // everything is already resolvable
     }
 
-    self.add_package_reqs(reqs).await.map(|_| true)
+    let pkg_reqs = pkg_json_remote_pkgs
+      .iter()
+      .map(|pkg| pkg.req.clone())
+      .collect::<Vec<_>>();
+    self.add_package_reqs(&pkg_reqs).await.map(|_| true)
   }
 
   pub async fn cache_package_info(

cli/npm/managed/resolvers/local.rs

@@ -672,7 +672,7 @@ async fn sync_resolution_with_fs(
     // but this is good enough for a first pass
     for workspace in pkg_json_deps_provider.workspace_pkgs() {
       symlink_package_dir(
-        &workspace.pkg_dir,
+        &workspace.target_dir,
         &root_node_modules_dir_path.join(&workspace.alias),
       )?;
     }

tests/specs/npm/npm_pkg_depend_dep_same_name/__test__.jsonc

@@ -0,0 +1,13 @@
+{
+  "tempDir": true,
+  "envs": {
+    "DENO_FUTURE": "1"
+  },
+  "steps": [{
+    "args": "install",
+    "output": "[WILDCARD]"
+  }, {
+    "args": "run index.ts",
+    "output": "3\n"
+  }]
+}

tests/specs/npm/npm_pkg_depend_dep_same_name/index.ts

@@ -0,0 +1,4 @@
+// this should resolve to the remote package and not itself
+import { add } from "@denotest/add";
+
+console.log(add(1, 2));

tests/specs/npm/npm_pkg_depend_dep_same_name/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "@denotest/add",
+  "version": "1.0.0",
+  "dependencies": {
+    "@denotest/add": "1.0.0"
+  }
+}