From 72f025c74320ffd6df9758ac53c74db68e0a3e10 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 9 Sep 2024 16:19:29 -0400 Subject: [PATCH] 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 | 34 +++++-------------- cli/npm/managed/resolvers/local.rs | 18 ++++++---- .../install/alias_deno_json/__test__.jsonc | 10 ------ tests/specs/install/alias_deno_json/deno.json | 5 --- .../install/alias_deno_json/package.json | 2 -- tests/specs/install/alias_deno_json/verify.ts | 2 -- .../alias_invalid_path_char/__test__.jsonc | 10 ------ .../alias_invalid_path_char/deno.jsonc | 7 ---- .../alias_invalid_path_char/package.json | 2 -- .../install/alias_invalid_path_char/verify.ts | 10 ------ .../verify.out | 2 +- .../__test__.jsonc | 4 +-- .../member/main.js | 6 ---- 13 files changed, 24 insertions(+), 88 deletions(-) delete mode 100644 tests/specs/install/alias_deno_json/__test__.jsonc delete mode 100644 tests/specs/install/alias_deno_json/deno.json delete mode 100644 tests/specs/install/alias_deno_json/package.json delete mode 100644 tests/specs/install/alias_deno_json/verify.ts delete mode 100644 tests/specs/install/alias_invalid_path_char/__test__.jsonc delete mode 100644 tests/specs/install/alias_invalid_path_char/deno.jsonc delete mode 100644 tests/specs/install/alias_invalid_path_char/package.json delete mode 100644 tests/specs/install/alias_invalid_path_char/verify.ts diff --git a/cli/args/package_json.rs b/cli/args/package_json.rs index b9f0919d5d..2529e54fdf 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, pub base_dir: PathBuf, pub req: PackageReq, } #[derive(Debug)] pub struct InstallNpmWorkspacePkg { - pub alias: String, + pub alias: Option, 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 d0fd523e22..472bf2a47d 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 0398e2eeb5..0000000000 --- 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 a1adfb35e2..0000000000 --- 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 2c63c08510..0000000000 --- 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 ea6cadb709..0000000000 --- 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 4a30586350..0000000000 --- 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 85befb55e8..0000000000 --- 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 2c63c08510..0000000000 --- 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 497e1d8dd9..0000000000 --- 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 bc6cb31f7c..8c989abd4a 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 1bad68fd4e..d0d7579b87 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 0f64cf5532..1ca631410f 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, -);