fix: remove recently added deno.json node_modules aliasing

cli/args/package_json.rs

@@ -1,6 +1,5 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
-use std::collections::HashSet;
 use std::path::PathBuf;
 use std::sync::Arc;
 
@@ -10,18 +9,16 @@ use deno_package_json::PackageJsonDepValue;
 use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageReq;
 
-use crate::util::path::is_banned_path_char;
-
 #[derive(Debug)]
 pub struct InstallNpmRemotePkg {
-  pub alias: String,
+  pub alias: Option<String>,
   pub base_dir: PathBuf,
   pub req: PackageReq,
 }
 
 #[derive(Debug)]
 pub struct InstallNpmWorkspacePkg {
-  pub alias: String,
+  pub alias: Option<String>,
   pub target_dir: PathBuf,
 }
 
@@ -43,49 +40,41 @@ impl NpmInstallDepsProvider {
     let workspace_npm_pkgs = workspace.npm_packages();
 
     for (_, folder) in workspace.config_folders() {
-      let mut deno_json_aliases = HashSet::new();
-
       // deal with the deno.json first because it takes precedence during resolution
       if let Some(deno_json) = &folder.deno_json {
         // don't bother with externally referenced import maps as users
         // should inline their import map to get this behaviour
         if let Some(serde_json::Value::Object(obj)) = &deno_json.json.imports {
-          deno_json_aliases.reserve(obj.len());
           let mut pkg_pkgs = Vec::with_capacity(obj.len());
-          for (alias, value) in obj {
+          for (_alias, value) in obj {
             let serde_json::Value::String(specifier) = value else {
               continue;
             };
             let Ok(npm_req_ref) = NpmPackageReqReference::from_str(specifier)
             else {
               continue;
             };
-            // skip any aliases with banned characters
-            if alias.chars().any(|c| c == '\\' || is_banned_path_char(c)) {
-              continue;
-            }
-            deno_json_aliases.insert(alias.to_lowercase());
             let pkg_req = npm_req_ref.into_inner().req;
             let workspace_pkg = workspace_npm_pkgs
               .iter()
               .find(|pkg| pkg.matches_req(&pkg_req));
 
             if let Some(pkg) = workspace_pkg {
               workspace_pkgs.push(InstallNpmWorkspacePkg {
-                alias: alias.to_string(),
+                alias: None,
                 target_dir: pkg.pkg_json.dir_path().to_path_buf(),
               });
             } else {
               pkg_pkgs.push(InstallNpmRemotePkg {
-                alias: alias.to_string(),
+                alias: None,
                 base_dir: deno_json.dir_path(),
                 req: pkg_req,
               });
             }
           }
 
           // sort within each package (more like npm resolution)
-          pkg_pkgs.sort_by(|a, b| a.alias.cmp(&b.alias));
+          pkg_pkgs.sort_by(|a, b| a.req.cmp(&b.req));
           remote_pkgs.extend(pkg_pkgs);
         }
       }
@@ -97,11 +86,6 @@ impl NpmInstallDepsProvider {
           let Ok(dep) = dep else {
             continue;
           };
-          if deno_json_aliases.contains(&alias.to_lowercase()) {
-            // aliases in deno.json take precedence over package.json, so
-            // since this can't be resolved don't bother installing it
-            continue;
-          }
           match dep {
             PackageJsonDepValue::Req(pkg_req) => {
               let workspace_pkg = workspace_npm_pkgs.iter().find(|pkg| {
@@ -112,12 +96,12 @@ impl NpmInstallDepsProvider {
 
               if let Some(pkg) = workspace_pkg {
                 workspace_pkgs.push(InstallNpmWorkspacePkg {
-                  alias,
+                  alias: Some(alias),
                   target_dir: pkg.pkg_json.dir_path().to_path_buf(),
                 });
               } else {
                 pkg_pkgs.push(InstallNpmRemotePkg {
-                  alias,
+                  alias: Some(alias),
                   base_dir: pkg_json.dir_path().to_path_buf(),
                   req: pkg_req,
                 });
@@ -128,7 +112,7 @@ impl NpmInstallDepsProvider {
                 pkg.matches_name_and_version_req(&alias, &version_req)
               }) {
                 workspace_pkgs.push(InstallNpmWorkspacePkg {
-                  alias,
+                  alias: Some(alias),
                   target_dir: pkg.pkg_json.dir_path().to_path_buf(),
                 });
               }

cli/npm/managed/resolvers/local.rs

@@ -642,13 +642,16 @@ async fn sync_resolution_with_fs(
       } else {
         continue; // skip, package not found
       };
-      let alias_clashes = remote.req.name != remote.alias
-        && newest_packages_by_name.contains_key(&remote.alias);
+      let Some(remote_alias) = &remote.alias else {
+        continue;
+      };
+      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) {
+        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)
@@ -679,7 +682,7 @@ async fn sync_resolution_with_fs(
           existing_child_node_modules_dirs.insert(dest_node_modules.clone());
         }
         let mut dest_path = dest_node_modules;
-        dest_path.push(&remote.alias);
+        dest_path.push(remote_alias);
 
         symlink_package_dir(&local_registry_package_path, &dest_path)?;
       } else {
@@ -689,7 +692,7 @@ async fn sync_resolution_with_fs(
         {
           symlink_package_dir(
             &local_registry_package_path,
-            &join_package_name(root_node_modules_dir_path, &remote.alias),
+            &join_package_name(root_node_modules_dir_path, remote_alias),
           )?;
         }
       }
@@ -774,9 +777,12 @@ async fn sync_resolution_with_fs(
     // install correctly for a workspace (potentially in sub directories),
     // but this is good enough for a first pass
     for workspace in npm_install_deps_provider.workspace_pkgs() {
+      let Some(workspace_alias) = &workspace.alias else {
+        continue;
+      };
       symlink_package_dir(
         &workspace.target_dir,
-        &root_node_modules_dir_path.join(&workspace.alias),
+        &root_node_modules_dir_path.join(workspace_alias),
       )?;
     }
   }

tests/specs/install/alias_pkg_json_and_deno_json_npm_pkg/verify.out

@@ -1,4 +1,4 @@
-@denotest/add
+@denotest/esm-basic
 [Module: null prototype] {
   add: [Function (anonymous)],
   default: { add: [Function (anonymous)] }

tests/specs/workspaces/nested_deno_pkg_npm_conflict/__test__.jsonc

@@ -5,9 +5,9 @@
     "output": "[WILDCARD]"
   }, {
     "args": "run --allow-read main.js",
-    "output": "4\n0.5.0\n"
+    "output": "4\n1.0.0\n"
   }, {
     "args": "run --allow-read member/main.js",
-    "output": "3\n1.0.0\n"
+    "output": "3\n"
   }]
 }

tests/specs/workspaces/nested_deno_pkg_npm_conflict/member/main.js

@@ -1,9 +1,3 @@
 import { add } from "@denotest/add";
 
 console.log(add(1, 2));
-
-console.log(
-  JSON.parse(Deno.readTextFileSync(
-    new URL("node_modules/@denotest/add/package.json", import.meta.url),
-  )).version,
-);