fix(npm): improve peer dependency resolution (#17835)

This PR fixes peer dependency resolution to only resolve peers based on
the current graph traversal path. Previously, it would resolve a peers
by looking at a graph node's ancestors, which is not correct because
graph nodes are shared by different resolutions.

It also stores more information about peer dependency resolution in the
lockfile.

cli/npm/resolvers/common.rs

@@ -18,7 +18,7 @@ use crate::args::Lockfile;
 use crate::npm::cache::should_sync_download;
 use crate::npm::resolution::NpmResolutionSnapshot;
 use crate::npm::NpmCache;
-use crate::npm::NpmPackageNodeId;
+use crate::npm::NpmPackageId;
 use crate::npm::NpmResolutionPackage;
 
 pub trait InnerNpmPackageResolver: Send + Sync {
@@ -39,10 +39,7 @@ pub trait InnerNpmPackageResolver: Send + Sync {
     specifier: &ModuleSpecifier,
   ) -> Result<PathBuf, AnyError>;
 
-  fn package_size(
-    &self,
-    package_id: &NpmPackageNodeId,
-  ) -> Result<u64, AnyError>;
+  fn package_size(&self, package_id: &NpmPackageId) -> Result<u64, AnyError>;
 
   fn has_packages(&self) -> bool;
 
@@ -79,7 +76,7 @@ pub async fn cache_packages(
   if sync_download {
     // we're running the tests not with --quiet
     // and we want the output to be deterministic
-    packages.sort_by(|a, b| a.id.cmp(&b.id));
+    packages.sort_by(|a, b| a.pkg_id.cmp(&b.pkg_id));
   }
 
   let mut handles = Vec::with_capacity(packages.len());
@@ -90,7 +87,7 @@ pub async fn cache_packages(
     let handle = tokio::task::spawn(async move {
       cache
         .ensure_package(
-          (package.id.name.as_str(), &package.id.version),
+          (package.pkg_id.nv.name.as_str(), &package.pkg_id.nv.version),
           &package.dist,
           &registry_url,
         )

cli/npm/resolvers/global.rs

@@ -22,9 +22,9 @@ use crate::npm::resolution::NpmResolution;
 use crate::npm::resolution::NpmResolutionSnapshot;
 use crate::npm::resolvers::common::cache_packages;
 use crate::npm::NpmCache;
-use crate::npm::NpmPackageNodeId;
+use crate::npm::NpmPackageId;
+use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolutionPackage;
-use crate::npm::RealNpmRegistryApi;
 
 use super::common::ensure_registry_read_permission;
 use super::common::types_package_name;
@@ -41,7 +41,7 @@ pub struct GlobalNpmPackageResolver {
 impl GlobalNpmPackageResolver {
   pub fn new(
     cache: NpmCache,
-    api: RealNpmRegistryApi,
+    api: NpmRegistryApi,
     initial_snapshot: Option<NpmResolutionSnapshot>,
   ) -> Self {
     let registry_url = api.base_url().to_owned();
@@ -54,7 +54,7 @@ impl GlobalNpmPackageResolver {
     }
   }
 
-  fn package_folder(&self, id: &NpmPackageNodeId) -> PathBuf {
+  fn package_folder(&self, id: &NpmPackageId) -> PathBuf {
     let folder_id = self
       .resolution
       .resolve_package_cache_folder_id_from_id(id)
@@ -82,7 +82,7 @@ impl InnerNpmPackageResolver for GlobalNpmPackageResolver {
     pkg_req: &NpmPackageReq,
   ) -> Result<PathBuf, AnyError> {
     let pkg = self.resolution.resolve_package_from_deno_module(pkg_req)?;
-    Ok(self.package_folder(&pkg.id))
+    Ok(self.package_folder(&pkg.pkg_id))
   }
 
   fn resolve_package_folder_from_package(
@@ -107,7 +107,7 @@ impl InnerNpmPackageResolver for GlobalNpmPackageResolver {
         .resolution
         .resolve_package_from_package(name, &referrer_pkg_id)?
     };
-    Ok(self.package_folder(&pkg.id))
+    Ok(self.package_folder(&pkg.pkg_id))
   }
 
   fn resolve_package_folder_from_specifier(
@@ -125,10 +125,7 @@ impl InnerNpmPackageResolver for GlobalNpmPackageResolver {
     )
   }
 
-  fn package_size(
-    &self,
-    package_id: &NpmPackageNodeId,
-  ) -> Result<u64, AnyError> {
+  fn package_size(&self, package_id: &NpmPackageId) -> Result<u64, AnyError> {
     let package_folder = self.package_folder(package_id);
     Ok(crate::util::fs::dir_size(&package_folder)?)
   }

cli/npm/resolvers/local.rs

@@ -32,9 +32,9 @@ use crate::npm::cache::NpmPackageCacheFolderId;
 use crate::npm::resolution::NpmResolution;
 use crate::npm::resolution::NpmResolutionSnapshot;
 use crate::npm::NpmCache;
-use crate::npm::NpmPackageNodeId;
+use crate::npm::NpmPackageId;
+use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolutionPackage;
-use crate::npm::RealNpmRegistryApi;
 use crate::util::fs::copy_dir_recursive;
 use crate::util::fs::hard_link_dir_recursive;
 
@@ -56,7 +56,7 @@ pub struct LocalNpmPackageResolver {
 impl LocalNpmPackageResolver {
   pub fn new(
     cache: NpmCache,
-    api: RealNpmRegistryApi,
+    api: NpmRegistryApi,
     node_modules_folder: PathBuf,
     initial_snapshot: Option<NpmResolutionSnapshot>,
   ) -> Self {
@@ -112,7 +112,7 @@ impl LocalNpmPackageResolver {
 
   fn get_package_id_folder(
     &self,
-    package_id: &NpmPackageNodeId,
+    package_id: &NpmPackageId,
   ) -> Result<PathBuf, AnyError> {
     match self.resolution.resolve_package_from_id(package_id) {
       Some(package) => Ok(self.get_package_id_folder_from_package(&package)),
@@ -136,7 +136,7 @@ impl LocalNpmPackageResolver {
         &package.get_package_cache_folder_id(),
       ))
       .join("node_modules")
-      .join(&package.id.name)
+      .join(&package.pkg_id.nv.name)
   }
 }
 
@@ -203,10 +203,7 @@ impl InnerNpmPackageResolver for LocalNpmPackageResolver {
     Ok(package_root_path)
   }
 
-  fn package_size(
-    &self,
-    package_id: &NpmPackageNodeId,
-  ) -> Result<u64, AnyError> {
+  fn package_size(&self, package_id: &NpmPackageId) -> Result<u64, AnyError> {
     let package_folder_path = self.get_package_id_folder(package_id)?;
 
     Ok(crate::util::fs::dir_size(&package_folder_path)?)
@@ -303,7 +300,9 @@ async fn sync_resolution_with_fs(
   if sync_download {
     // we're running the tests not with --quiet
     // and we want the output to be deterministic
-    package_partitions.packages.sort_by(|a, b| a.id.cmp(&b.id));
+    package_partitions
+      .packages
+      .sort_by(|a, b| a.pkg_id.cmp(&b.pkg_id));
   }
   let mut handles: Vec<JoinHandle<Result<(), AnyError>>> =
     Vec::with_capacity(package_partitions.packages.len());
@@ -314,7 +313,7 @@ async fn sync_resolution_with_fs(
     let initialized_file = folder_path.join(".initialized");
     if !cache
       .cache_setting()
-      .should_use_for_npm_package(&package.id.name)
+      .should_use_for_npm_package(&package.pkg_id.nv.name)
       || !initialized_file.exists()
     {
       let cache = cache.clone();
@@ -323,19 +322,19 @@ async fn sync_resolution_with_fs(
       let handle = tokio::task::spawn(async move {
         cache
           .ensure_package(
-            (&package.id.name, &package.id.version),
+            (&package.pkg_id.nv.name, &package.pkg_id.nv.version),
             &package.dist,
             &registry_url,
           )
           .await?;
         let sub_node_modules = folder_path.join("node_modules");
         let package_path =
-          join_package_name(&sub_node_modules, &package.id.name);
+          join_package_name(&sub_node_modules, &package.pkg_id.nv.name);
         fs::create_dir_all(&package_path)
           .with_context(|| format!("Creating '{}'", folder_path.display()))?;
         let cache_folder = cache.package_folder_for_name_and_version(
-          &package.id.name,
-          &package.id.version,
+          &package.pkg_id.nv.name,
+          &package.pkg_id.nv.version,
           &registry_url,
         );
         // for now copy, but in the future consider hard linking
@@ -365,7 +364,8 @@ async fn sync_resolution_with_fs(
     let initialized_file = destination_path.join(".initialized");
     if !initialized_file.exists() {
       let sub_node_modules = destination_path.join("node_modules");
-      let package_path = join_package_name(&sub_node_modules, &package.id.name);
+      let package_path =
+        join_package_name(&sub_node_modules, &package.pkg_id.nv.name);
       fs::create_dir_all(&package_path).with_context(|| {
         format!("Creating '{}'", destination_path.display())
       })?;
@@ -375,7 +375,7 @@ async fn sync_resolution_with_fs(
             &package_cache_folder_id.with_no_count(),
           ))
           .join("node_modules"),
-        &package.id.name,
+        &package.pkg_id.nv.name,
       );
       hard_link_dir_recursive(&source_path, &package_path)?;
       // write out a file that indicates this folder has been initialized
@@ -406,7 +406,7 @@ async fn sync_resolution_with_fs(
         &deno_local_registry_dir
           .join(dep_folder_name)
           .join("node_modules"),
-        &dep_id.name,
+        &dep_id.nv.name,
       );
       symlink_package_dir(
         &dep_folder_path,
@@ -428,10 +428,10 @@ async fn sync_resolution_with_fs(
       .map(|id| (id, true)),
   );
   while let Some((package_id, is_top_level)) = pending_packages.pop_front() {
-    let root_folder_name = if found_names.insert(package_id.name.clone()) {
-      package_id.name.clone()
+    let root_folder_name = if found_names.insert(package_id.nv.name.clone()) {
+      package_id.nv.name.clone()
     } else if is_top_level {
-      package_id.display()
+      format!("{}@{}", package_id.nv.name, package_id.nv.version)
     } else {
       continue; // skip, already handled
     };
@@ -442,7 +442,7 @@ async fn sync_resolution_with_fs(
           &package.get_package_cache_folder_id(),
         ))
         .join("node_modules"),
-      &package_id.name,
+      &package_id.nv.name,
     );
 
     symlink_package_dir(
@@ -457,18 +457,21 @@ async fn sync_resolution_with_fs(
   Ok(())
 }
 
-fn get_package_folder_id_folder_name(id: &NpmPackageCacheFolderId) -> String {
-  let copy_str = if id.copy_index == 0 {
+fn get_package_folder_id_folder_name(
+  folder_id: &NpmPackageCacheFolderId,
+) -> String {
+  let copy_str = if folder_id.copy_index == 0 {
     "".to_string()
   } else {
-    format!("_{}", id.copy_index)
+    format!("_{}", folder_id.copy_index)
   };
-  let name = if id.name.to_lowercase() == id.name {
-    Cow::Borrowed(&id.name)
+  let nv = &folder_id.nv;
+  let name = if nv.name.to_lowercase() == nv.name {
+    Cow::Borrowed(&nv.name)
   } else {
-    Cow::Owned(format!("_{}", mixed_case_package_name_encode(&id.name)))
+    Cow::Owned(format!("_{}", mixed_case_package_name_encode(&nv.name)))
   };
-  format!("{}@{}{}", name, id.version, copy_str).replace('/', "+")
+  format!("{}@{}{}", name, nv.version, copy_str).replace('/', "+")
 }
 
 fn symlink_package_dir(

cli/npm/resolvers/mod.rs

@@ -30,9 +30,9 @@ use crate::util::fs::canonicalize_path_maybe_not_exists;
 use self::common::InnerNpmPackageResolver;
 use self::local::LocalNpmPackageResolver;
 use super::NpmCache;
-use super::NpmPackageNodeId;
+use super::NpmPackageId;
+use super::NpmRegistryApi;
 use super::NpmResolutionSnapshot;
-use super::RealNpmRegistryApi;
 
 /// State provided to the process via an environment variable.
 #[derive(Clone, Debug, Serialize, Deserialize)]
@@ -46,7 +46,7 @@ pub struct NpmPackageResolver {
   no_npm: bool,
   inner: Arc<dyn InnerNpmPackageResolver>,
   local_node_modules_path: Option<PathBuf>,
-  api: RealNpmRegistryApi,
+  api: NpmRegistryApi,
   cache: NpmCache,
   maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
 }
@@ -62,13 +62,13 @@ impl std::fmt::Debug for NpmPackageResolver {
 }
 
 impl NpmPackageResolver {
-  pub fn new(cache: NpmCache, api: RealNpmRegistryApi) -> Self {
+  pub fn new(cache: NpmCache, api: NpmRegistryApi) -> Self {
     Self::new_inner(cache, api, false, None, None, None)
   }
 
   pub async fn new_with_maybe_lockfile(
     cache: NpmCache,
-    api: RealNpmRegistryApi,
+    api: NpmRegistryApi,
     no_npm: bool,
     local_node_modules_path: Option<PathBuf>,
     initial_snapshot: Option<NpmResolutionSnapshot>,
@@ -105,7 +105,7 @@ impl NpmPackageResolver {
 
   fn new_inner(
     cache: NpmCache,
-    api: RealNpmRegistryApi,
+    api: NpmRegistryApi,
     no_npm: bool,
     local_node_modules_path: Option<PathBuf>,
     maybe_snapshot: Option<NpmResolutionSnapshot>,
@@ -187,7 +187,7 @@ impl NpmPackageResolver {
   /// Attempts to get the package size in bytes.
   pub fn package_size(
     &self,
-    package_id: &NpmPackageNodeId,
+    package_id: &NpmPackageId,
   ) -> Result<u64, AnyError> {
     self.inner.package_size(package_id)
   }