fix: remove recently added deno.json node_modules aliasing (#25542)

This was initially added in #25399 in order to make transitioning over
from package.json to deno.json more easy, but it causes some problems
that are shown in the issue and it also means that the output of `deno
install` would have different resolution than `npm install`. Overall, I
think it's too much complexity to be smarter about this and it's
probably best to not do it. If someone needs an aliased folder then they
should keep using a package.json

Closes #25538

13 files changed, 24 insertions, 88 deletions

diff --git a/cli/args/package_json.rs b/cli/args/package_json.rs
index b9f0919d5..2529e54fd 100644
--- a/cli/args/package_json.rs
+++ b/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,16 +40,13 @@ 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;
             };
@@ -60,11 +54,6 @@ impl NpmInstallDepsProvider {
             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()
@@ -72,12 +61,12 @@ impl NpmInstallDepsProvider {
 
             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,
               });
@@ -85,7 +74,7 @@ impl NpmInstallDepsProvider {
           }
 
           // 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(),
                 });
               }
diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs
index d0fd523e2..472bf2a47 100644
--- a/cli/npm/managed/resolvers/local.rs
+++ b/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),
       )?;
     }
   }
diff --git a/tests/specs/install/alias_deno_json/__test__.jsonc b/tests/specs/install/alias_deno_json/__test__.jsonc
deleted file mode 100644
index 0398e2eeb..000000000
--- a/tests/specs/install/alias_deno_json/__test__.jsonc
+++ /dev/null
@@ -1,10 +0,0 @@
-{
-  "tempDir": true,
-  "steps": [{
-    "args": "install",
-    "output": "[WILDCARD]"
-  }, {
-    "args": "run --allow-read=. verify.ts",
-    "output": "true\n"
-  }]
-}
diff --git a/tests/specs/install/alias_deno_json/deno.json b/tests/specs/install/alias_deno_json/deno.json
deleted file mode 100644
index a1adfb35e..000000000
--- a/tests/specs/install/alias_deno_json/deno.json
+++ /dev/null
@@ -1,5 +0,0 @@
-{
-  "imports": {
-    "alias": "npm:@denotest/add"
-  }
-}
diff --git a/tests/specs/install/alias_deno_json/package.json b/tests/specs/install/alias_deno_json/package.json
deleted file mode 100644
index 2c63c0851..000000000
--- a/tests/specs/install/alias_deno_json/package.json
+++ /dev/null
@@ -1,2 +0,0 @@
-{
-}
diff --git a/tests/specs/install/alias_deno_json/verify.ts b/tests/specs/install/alias_deno_json/verify.ts
deleted file mode 100644
index ea6cadb70..000000000
--- a/tests/specs/install/alias_deno_json/verify.ts
+++ /dev/null
@@ -1,2 +0,0 @@
-const stat = Deno.statSync(new URL("./node_modules/alias", import.meta.url));
-console.log(stat.isDirectory);
diff --git a/tests/specs/install/alias_invalid_path_char/__test__.jsonc b/tests/specs/install/alias_invalid_path_char/__test__.jsonc
deleted file mode 100644
index 4a3058635..000000000
--- a/tests/specs/install/alias_invalid_path_char/__test__.jsonc
+++ /dev/null
@@ -1,10 +0,0 @@
-{
-  "tempDir": true,
-  "steps": [{
-    "args": "install",
-    "output": "[WILDCARD]"
-  }, {
-    "args": "run --allow-read=. verify.ts",
-    "output": ".bin\n.deno\n@denotest\n"
-  }]
-}
diff --git a/tests/specs/install/alias_invalid_path_char/deno.jsonc b/tests/specs/install/alias_invalid_path_char/deno.jsonc
deleted file mode 100644
index 85befb55e..000000000
--- a/tests/specs/install/alias_invalid_path_char/deno.jsonc
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "imports": {
-    // alias*test is an invalid path char on windows, so
-    // don't create an alias for this
-    "alias*test": "npm:@denotest/add"
-  }
-}
diff --git a/tests/specs/install/alias_invalid_path_char/package.json b/tests/specs/install/alias_invalid_path_char/package.json
deleted file mode 100644
index 2c63c0851..000000000
--- a/tests/specs/install/alias_invalid_path_char/package.json
+++ /dev/null
@@ -1,2 +0,0 @@
-{
-}
diff --git a/tests/specs/install/alias_invalid_path_char/verify.ts b/tests/specs/install/alias_invalid_path_char/verify.ts
deleted file mode 100644
index 497e1d8dd..000000000
--- a/tests/specs/install/alias_invalid_path_char/verify.ts
+++ /dev/null
@@ -1,10 +0,0 @@
-const entries = Array.from(
-  Deno.readDirSync(new URL("./node_modules", import.meta.url)),
-);
-const names = entries.map((entry) => entry.name);
-names.sort();
-
-// won't have the invalid path alias
-for (const name of names) {
-  console.log(name);
-}
diff --git a/tests/specs/install/alias_pkg_json_and_deno_json_npm_pkg/verify.out b/tests/specs/install/alias_pkg_json_and_deno_json_npm_pkg/verify.out
index bc6cb31f7..8c989abd4 100644
--- a/tests/specs/install/alias_pkg_json_and_deno_json_npm_pkg/verify.out
+++ b/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)] }
diff --git a/tests/specs/workspaces/nested_deno_pkg_npm_conflict/__test__.jsonc b/tests/specs/workspaces/nested_deno_pkg_npm_conflict/__test__.jsonc
index 1bad68fd4..d0d7579b8 100644
--- a/tests/specs/workspaces/nested_deno_pkg_npm_conflict/__test__.jsonc
+++ b/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"
   }]
 }
diff --git a/tests/specs/workspaces/nested_deno_pkg_npm_conflict/member/main.js b/tests/specs/workspaces/nested_deno_pkg_npm_conflict/member/main.js
index 0f64cf553..1ca631410 100644
--- a/tests/specs/workspaces/nested_deno_pkg_npm_conflict/member/main.js
+++ b/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,
-);