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

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(),
                 });
               }

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_deno_json/__test__.jsonc

@@ -1,10 +0,0 @@
-{
-  "tempDir": true,
-  "steps": [{
-    "args": "install",
-    "output": "[WILDCARD]"
-  }, {
-    "args": "run --allow-read=. verify.ts",
-    "output": "true\n"
-  }]
-}

tests/specs/install/alias_deno_json/deno.json

@@ -1,5 +0,0 @@
-{
-  "imports": {
-    "alias": "npm:@denotest/add"
-  }
-}

tests/specs/install/alias_deno_json/package.json

@@ -1,2 +0,0 @@
-{
-}

tests/specs/install/alias_deno_json/verify.ts

@@ -1,2 +0,0 @@
-const stat = Deno.statSync(new URL("./node_modules/alias", import.meta.url));
-console.log(stat.isDirectory);

tests/specs/install/alias_invalid_path_char/__test__.jsonc

@@ -1,10 +0,0 @@
-{
-  "tempDir": true,
-  "steps": [{
-    "args": "install",
-    "output": "[WILDCARD]"
-  }, {
-    "args": "run --allow-read=. verify.ts",
-    "output": ".bin\n.deno\n@denotest\n"
-  }]
-}

tests/specs/install/alias_invalid_path_char/deno.jsonc

@@ -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"
-  }
-}

tests/specs/install/alias_invalid_path_char/package.json

@@ -1,2 +0,0 @@
-{
-}

tests/specs/install/alias_invalid_path_char/verify.ts

@@ -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);
-}

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,
-);