From 29186d7e5963f2398b28ee2c043b27e4881075ef Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 15 Jul 2024 15:08:51 -0400 Subject: [PATCH] fix(workspace): do not resolve to self for npm pkg depending on matching req (#24591) Closes #24584 --- cli/args/package_json.rs | 60 ++++++++++++------- cli/npm/managed/mod.rs | 20 ++++--- cli/npm/managed/resolvers/local.rs | 2 +- .../__test__.jsonc | 13 ++++ .../npm/npm_pkg_depend_dep_same_name/index.ts | 4 ++ .../npm_pkg_depend_dep_same_name/package.json | 7 +++ 6 files changed, 77 insertions(+), 29 deletions(-) create mode 100644 tests/specs/npm/npm_pkg_depend_dep_same_name/__test__.jsonc create mode 100644 tests/specs/npm/npm_pkg_depend_dep_same_name/index.ts create mode 100644 tests/specs/npm/npm_pkg_depend_dep_same_name/package.json diff --git a/cli/args/package_json.rs b/cli/args/package_json.rs index eb1c41c5df..170f8b6770 100644 --- a/cli/args/package_json.rs +++ b/cli/args/package_json.rs @@ -8,17 +8,26 @@ use deno_config::workspace::Workspace; use deno_semver::package::PackageReq; #[derive(Debug)] -pub struct InstallNpmWorkspacePkg { +pub struct InstallNpmRemotePkg { pub alias: String, - pub pkg_dir: PathBuf, + // todo(24419): use this when setting up the node_modules dir + #[allow(dead_code)] + pub base_dir: PathBuf, + pub req: PackageReq, +} + +#[derive(Debug)] +pub struct InstallNpmWorkspacePkg { + pub alias: String, + // todo(24419): use this when setting up the node_modules dir + #[allow(dead_code)] + pub base_dir: PathBuf, + pub target_dir: PathBuf, } -// todo(#24419): this is not correct, but it's good enough for now. -// We need deno_npm to be able to understand workspace packages and -// then have a way to properly lay them out on the file system #[derive(Debug, Default)] pub struct PackageJsonInstallDepsProvider { - remote_pkg_reqs: Vec, + remote_pkgs: Vec, workspace_pkgs: Vec, } @@ -29,27 +38,35 @@ impl PackageJsonInstallDepsProvider { pub fn from_workspace(workspace: &Arc) -> Self { let mut workspace_pkgs = Vec::new(); - let mut remote_pkg_reqs = Vec::new(); + let mut remote_pkgs = Vec::new(); let workspace_npm_pkgs = workspace.npm_packages(); for pkg_json in workspace.package_jsons() { let deps = pkg_json.resolve_local_package_json_deps(); - let mut pkg_reqs = Vec::with_capacity(deps.len()); + let mut pkg_pkgs = Vec::with_capacity(deps.len()); for (alias, dep) in deps { let Ok(dep) = dep else { continue; }; match dep { PackageJsonDepValue::Req(pkg_req) => { - if let Some(pkg) = workspace_npm_pkgs - .iter() - .find(|pkg| pkg.matches_req(&pkg_req)) - { + let workspace_pkg = workspace_npm_pkgs.iter().find(|pkg| { + pkg.matches_req(&pkg_req) + // do not resolve to the current package + && pkg.pkg_json.path != pkg_json.path + }); + + if let Some(pkg) = workspace_pkg { workspace_pkgs.push(InstallNpmWorkspacePkg { alias, - pkg_dir: pkg.pkg_json.dir_path().to_path_buf(), + base_dir: pkg_json.dir_path().to_path_buf(), + target_dir: pkg.pkg_json.dir_path().to_path_buf(), }); } else { - pkg_reqs.push(pkg_req) + pkg_pkgs.push(InstallNpmRemotePkg { + alias, + base_dir: pkg_json.dir_path().to_path_buf(), + req: pkg_req, + }); } } PackageJsonDepValue::Workspace(version_req) => { @@ -58,27 +75,28 @@ impl PackageJsonInstallDepsProvider { }) { workspace_pkgs.push(InstallNpmWorkspacePkg { alias, - pkg_dir: pkg.pkg_json.dir_path().to_path_buf(), + base_dir: pkg_json.dir_path().to_path_buf(), + target_dir: pkg.pkg_json.dir_path().to_path_buf(), }); } } } } // sort within each package - pkg_reqs.sort(); + pkg_pkgs.sort_by(|a, b| a.alias.cmp(&b.alias)); - remote_pkg_reqs.extend(pkg_reqs); + remote_pkgs.extend(pkg_pkgs); } - remote_pkg_reqs.shrink_to_fit(); + remote_pkgs.shrink_to_fit(); workspace_pkgs.shrink_to_fit(); Self { - remote_pkg_reqs, + remote_pkgs, workspace_pkgs, } } - pub fn remote_pkg_reqs(&self) -> &Vec { - &self.remote_pkg_reqs + pub fn remote_pkgs(&self) -> &Vec { + &self.remote_pkgs } pub fn workspace_pkgs(&self) -> &Vec { diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index 76645d1d69..1020a57e91 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -475,24 +475,30 @@ impl ManagedCliNpmResolver { if !self.top_level_install_flag.raise() { return Ok(false); // already did this } - let reqs = self.package_json_deps_provider.remote_pkg_reqs(); - if reqs.is_empty() { + let pkg_json_remote_pkgs = self.package_json_deps_provider.remote_pkgs(); + if pkg_json_remote_pkgs.is_empty() { return Ok(false); } // check if something needs resolving before bothering to load all // the package information (which is slow) - if reqs - .iter() - .all(|req| self.resolution.resolve_pkg_id_from_pkg_req(req).is_ok()) - { + if pkg_json_remote_pkgs.iter().all(|pkg| { + self + .resolution + .resolve_pkg_id_from_pkg_req(&pkg.req) + .is_ok() + }) { log::debug!( "All package.json deps resolvable. Skipping top level install." ); return Ok(false); // everything is already resolvable } - self.add_package_reqs(reqs).await.map(|_| true) + let pkg_reqs = pkg_json_remote_pkgs + .iter() + .map(|pkg| pkg.req.clone()) + .collect::>(); + self.add_package_reqs(&pkg_reqs).await.map(|_| true) } pub async fn cache_package_info( diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 2c6d9ca427..937b2a8996 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -672,7 +672,7 @@ async fn sync_resolution_with_fs( // but this is good enough for a first pass for workspace in pkg_json_deps_provider.workspace_pkgs() { symlink_package_dir( - &workspace.pkg_dir, + &workspace.target_dir, &root_node_modules_dir_path.join(&workspace.alias), )?; } diff --git a/tests/specs/npm/npm_pkg_depend_dep_same_name/__test__.jsonc b/tests/specs/npm/npm_pkg_depend_dep_same_name/__test__.jsonc new file mode 100644 index 0000000000..d2f4cf6fb9 --- /dev/null +++ b/tests/specs/npm/npm_pkg_depend_dep_same_name/__test__.jsonc @@ -0,0 +1,13 @@ +{ + "tempDir": true, + "envs": { + "DENO_FUTURE": "1" + }, + "steps": [{ + "args": "install", + "output": "[WILDCARD]" + }, { + "args": "run index.ts", + "output": "3\n" + }] +} diff --git a/tests/specs/npm/npm_pkg_depend_dep_same_name/index.ts b/tests/specs/npm/npm_pkg_depend_dep_same_name/index.ts new file mode 100644 index 0000000000..6d0d71ad2c --- /dev/null +++ b/tests/specs/npm/npm_pkg_depend_dep_same_name/index.ts @@ -0,0 +1,4 @@ +// this should resolve to the remote package and not itself +import { add } from "@denotest/add"; + +console.log(add(1, 2)); diff --git a/tests/specs/npm/npm_pkg_depend_dep_same_name/package.json b/tests/specs/npm/npm_pkg_depend_dep_same_name/package.json new file mode 100644 index 0000000000..8a5140e031 --- /dev/null +++ b/tests/specs/npm/npm_pkg_depend_dep_same_name/package.json @@ -0,0 +1,7 @@ +{ + "name": "@denotest/add", + "version": "1.0.0", + "dependencies": { + "@denotest/add": "1.0.0" + } +}