fix(cli): Create child node_modules for conflicting dependency versions, respect aliases in package.json (#24609)

Fixes #24419.

cli/npm/managed/resolvers/local.rs

@@ -7,9 +7,9 @@ mod bin_entries;
 use std::borrow::Cow;
 use std::cell::RefCell;
 use std::cmp::Ordering;
+use std::collections::hash_map::Entry;
 use std::collections::BTreeMap;
 use std::collections::HashMap;
-use std::collections::HashSet;
 use std::fs;
 use std::path::Path;
 use std::path::PathBuf;
@@ -609,16 +609,81 @@ async fn sync_resolution_with_fs(
     }
   }
 
-  // 4. Create all the top level packages in the node_modules folder, which are symlinks.
-  //
-  // Symlink node_modules/<package_name> to
-  // node_modules/.deno/<package_id>/node_modules/<package_name>
-  let mut found_names = HashSet::new();
-  let mut ids = snapshot.top_level_packages().collect::<Vec<_>>();
+  let mut found_names: HashMap<&String, &PackageNv> = HashMap::new();
+
+  // 4. Create symlinks for package json dependencies
+  {
+    for remote in pkg_json_deps_provider.remote_pkgs() {
+      let Some(remote_id) = snapshot
+        .resolve_best_package_id(&remote.req.name, &remote.req.version_req)
+      else {
+        continue; // skip, package not found
+      };
+      let remote_pkg = snapshot.package_from_id(&remote_id).unwrap();
+      let alias_clashes = remote.req.name != remote.alias
+        && newest_packages_by_name.contains_key(&remote.alias);
+      let install_in_child = {
+        // we'll install in the child if the alias is taken by another package, or
+        // if there's already a package with the same name but different version
+        // linked into the root
+        match found_names.entry(&remote.alias) {
+          Entry::Occupied(nv) => {
+            alias_clashes
+              || remote.req.name != nv.get().name // alias to a different package (in case of duplicate aliases)
+              || !remote.req.version_req.matches(&nv.get().version) // incompatible version
+          }
+          Entry::Vacant(entry) => {
+            entry.insert(&remote_pkg.id.nv);
+            alias_clashes
+          }
+        }
+      };
+      let target_folder_name = get_package_folder_id_folder_name(
+        &remote_pkg.get_package_cache_folder_id(),
+      );
+      let local_registry_package_path = join_package_name(
+        &deno_local_registry_dir
+          .join(&target_folder_name)
+          .join("node_modules"),
+        &remote_pkg.id.nv.name,
+      );
+      if install_in_child {
+        // symlink the dep into the package's child node_modules folder
+        let dest_path =
+          remote.base_dir.join("node_modules").join(&remote.alias);
+
+        symlink_package_dir(&local_registry_package_path, &dest_path)?;
+      } else {
+        // symlink the package into `node_modules/<alias>`
+        if setup_cache
+          .insert_root_symlink(&remote_pkg.id.nv.name, &target_folder_name)
+        {
+          symlink_package_dir(
+            &local_registry_package_path,
+            &join_package_name(root_node_modules_dir_path, &remote.alias),
+          )?;
+        }
+      }
+    }
+  }
+
+  // 5. Create symlinks for the remaining top level packages in the node_modules folder.
+  // (These may be present if they are not in the package.json dependencies, such as )
+  // Symlink node_modules/.deno/<package_id>/node_modules/<package_name> to
+  // node_modules/<package_name>
+  let mut ids = snapshot
+    .top_level_packages()
+    .filter(|f| !found_names.contains_key(&f.nv.name))
+    .collect::<Vec<_>>();
   ids.sort_by(|a, b| b.cmp(a)); // create determinism and only include the latest version
   for id in ids {
-    if !found_names.insert(&id.nv.name) {
-      continue; // skip, already handled
+    match found_names.entry(&id.nv.name) {
+      Entry::Occupied(_) => {
+        continue; // skip, already handled
+      }
+      Entry::Vacant(entry) => {
+        entry.insert(&id.nv);
+      }
     }
     let package = snapshot.package_from_id(id).unwrap();
     let target_folder_name =
@@ -638,11 +703,16 @@ async fn sync_resolution_with_fs(
     }
   }
 
-  // 5. Create a node_modules/.deno/node_modules/<package-name> directory with
+  // 6. Create a node_modules/.deno/node_modules/<package-name> directory with
   // the remaining packages
   for package in newest_packages_by_name.values() {
-    if !found_names.insert(&package.id.nv.name) {
-      continue; // skip, already handled
+    match found_names.entry(&package.id.nv.name) {
+      Entry::Occupied(_) => {
+        continue; // skip, already handled
+      }
+      Entry::Vacant(entry) => {
+        entry.insert(&package.id.nv);
+      }
     }
 
     let target_folder_name =
@@ -663,13 +733,13 @@ async fn sync_resolution_with_fs(
     }
   }
 
-  // 6. Set up `node_modules/.bin` entries for packages that need it.
+  // 7. Set up `node_modules/.bin` entries for packages that need it.
   {
     let bin_entries = std::mem::take(&mut *bin_entries.borrow_mut());
     bin_entries.finish(snapshot, &bin_node_modules_dir_path)?;
   }
 
-  // 7. Create symlinks for the workspace packages
+  // 8. Create symlinks for the workspace packages
   {
     // todo(#24419): this is not exactly correct because it should
     // install correctly for a workspace (potentially in sub directories),

tests/registry/npm/@denotest/add/0.5.0/index.d.ts

@@ -0,0 +1 @@
+export function sum(a: number, b: number): number;

tests/registry/npm/@denotest/add/0.5.0/index.js

@@ -0,0 +1 @@
+module.exports.sum = (a, b) => a + b;

tests/registry/npm/@denotest/add/0.5.0/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "@denotest/add",
+  "version": "0.5.0"
+}

tests/specs/npm/workspace_conflicting_dep/__test__.jsonc

@@ -0,0 +1,24 @@
+{
+  "tempDir": true,
+  "tests": {
+    "conflicting_deps": {
+      "envs": {
+        "DENO_FUTURE": "1"
+      },
+      "steps": [
+        {
+          "args": "install",
+          "output": "[WILDCARD]"
+        },
+        {
+          "args": "run ./a/index.js",
+          "output": "1 + 2 = 3\n"
+        },
+        {
+          "args": "run ./b/index.js",
+          "output": "1 + 2 = 3\n"
+        }
+      ]
+    }
+  }
+}

tests/specs/npm/workspace_conflicting_dep/a/index.js

@@ -0,0 +1,3 @@
+import { sum } from "@denotest/add";
+
+console.log(`1 + 2 = ${sum(1, 2)}`);

tests/specs/npm/workspace_conflicting_dep/a/package.json

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

tests/specs/npm/workspace_conflicting_dep/b/index.js

@@ -0,0 +1,3 @@
+import { add } from "@denotest/add";
+
+console.log(`1 + 2 = ${add(1, 2)}`);

tests/specs/npm/workspace_conflicting_dep/b/package.json

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

tests/specs/npm/workspace_conflicting_dep/package.json

@@ -0,0 +1,6 @@
+{
+  "workspaces": [
+    "./a",
+    "./b"
+  ]
+}

tests/specs/npm/workspace_dep_aliases/__test__.jsonc

@@ -0,0 +1,24 @@
+{
+  "tempDir": true,
+  "tests": {
+    "conflicting_deps": {
+      "envs": {
+        "DENO_FUTURE": "1"
+      },
+      "steps": [
+        {
+          "args": "install",
+          "output": "[WILDCARD]"
+        },
+        {
+          "args": "run ./a/index.js",
+          "output": "1 + 2 = 3\n"
+        },
+        {
+          "args": "run ./b/index.js",
+          "output": "1 + 2 = 3\n"
+        }
+      ]
+    }
+  }
+}

tests/specs/npm/workspace_dep_aliases/a/index.js

@@ -0,0 +1,3 @@
+import { sum } from "sumPkg";
+
+console.log(`1 + 2 = ${sum(1, 2)}`);

tests/specs/npm/workspace_dep_aliases/a/package.json

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

tests/specs/npm/workspace_dep_aliases/b/index.js

@@ -0,0 +1,3 @@
+import { add } from "addPkg";
+
+console.log(`1 + 2 = ${add(1, 2)}`);

tests/specs/npm/workspace_dep_aliases/b/package.json

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

tests/specs/npm/workspace_dep_aliases/package.json

@@ -0,0 +1,6 @@
+{
+  "workspaces": [
+    "./a",
+    "./b"
+  ]
+}