fix(npm): do not "npm install" when npm specifier happens to match package.json entry (#18660)

cli/graph_util.rs

@@ -187,6 +187,7 @@ pub async fn create_graph_and_maybe_check(
   let mut graph = ModuleGraph::default();
   build_graph_with_npm_resolution(
     &mut graph,
+    &cli_resolver,
     &ps.npm_resolver,
     roots,
     &mut cache,
@@ -249,6 +250,7 @@ pub async fn create_graph_and_maybe_check(
 
 pub async fn build_graph_with_npm_resolution<'a>(
   graph: &mut ModuleGraph,
+  cli_graph_resolver: &CliGraphResolver,
   npm_resolver: &NpmPackageResolver,
   roots: Vec<ModuleSpecifier>,
   loader: &mut dyn deno_graph::source::Loader,
@@ -256,6 +258,12 @@ pub async fn build_graph_with_npm_resolution<'a>(
 ) -> Result<(), AnyError> {
   graph.build(roots, loader, options).await;
 
+  // ensure that the top level package.json is installed if a
+  // specifier was matched in the package.json
+  cli_graph_resolver
+    .top_level_package_json_install_if_necessary()
+    .await?;
+
   // resolve the dependencies of any pending dependencies
   // that were inserted by building the graph
   npm_resolver.resolve_pending().await?;

cli/npm/installer.rs

@@ -1,6 +1,5 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
-use std::sync::atomic::AtomicBool;
 use std::sync::Arc;
 
 use deno_core::error::AnyError;
@@ -9,13 +8,14 @@ use deno_core::futures::StreamExt;
 use deno_npm::registry::NpmRegistryApi;
 
 use crate::args::package_json::PackageJsonDeps;
+use crate::util::sync::AtomicFlag;
 
 use super::NpmRegistry;
 use super::NpmResolution;
 
 #[derive(Debug)]
 struct PackageJsonDepsInstallerInner {
-  has_installed: AtomicBool,
+  has_installed_flag: AtomicFlag,
   npm_registry_api: NpmRegistry,
   npm_resolution: NpmResolution,
   package_deps: PackageJsonDeps,
@@ -33,7 +33,7 @@ impl PackageJsonDepsInstaller {
   ) -> Self {
     Self(deps.map(|package_deps| {
       Arc::new(PackageJsonDepsInstallerInner {
-        has_installed: AtomicBool::new(false),
+        has_installed_flag: Default::default(),
         npm_registry_api,
         npm_resolution,
         package_deps,
@@ -45,30 +45,15 @@ impl PackageJsonDepsInstaller {
     self.0.as_ref().map(|inner| &inner.package_deps)
   }
 
-  /// Gets if the package.json has the specified package name.
-  pub fn has_package_name(&self, name: &str) -> bool {
-    if let Some(package_deps) = self.package_deps() {
-      // ensure this looks at the package name and not the
-      // bare specifiers (do not look at the keys!)
-      package_deps
-        .values()
-        .filter_map(|r| r.as_ref().ok())
-        .any(|v| v.name == name)
-    } else {
-      false
-    }
-  }
-
   /// Installs the top level dependencies in the package.json file
   /// without going through and resolving the descendant dependencies yet.
   pub async fn ensure_top_level_install(&self) -> Result<(), AnyError> {
-    use std::sync::atomic::Ordering;
     let inner = match &self.0 {
       Some(inner) => inner,
       None => return Ok(()),
     };
 
-    if inner.has_installed.swap(true, Ordering::SeqCst) {
+    if !inner.has_installed_flag.raise() {
       return Ok(()); // already installed by something else
     }
 

cli/npm/registry.rs

@@ -5,8 +5,6 @@ use std::collections::HashSet;
 use std::fs;
 use std::io::ErrorKind;
 use std::path::PathBuf;
-use std::sync::atomic::AtomicBool;
-use std::sync::atomic::Ordering;
 use std::sync::Arc;
 
 use async_trait::async_trait;
@@ -31,6 +29,7 @@ use crate::cache::CACHE_PERM;
 use crate::http_util::HttpClient;
 use crate::util::fs::atomic_write_file;
 use crate::util::progress_bar::ProgressBar;
+use crate::util::sync::AtomicFlag;
 
 use super::cache::should_sync_download;
 use super::cache::NpmCache;
@@ -70,7 +69,7 @@ impl NpmRegistry {
     Self(Some(Arc::new(NpmRegistryApiInner {
       base_url,
       cache,
-      force_reload: AtomicBool::new(false),
+      force_reload_flag: Default::default(),
       mem_cache: Default::default(),
       previously_reloaded_packages: Default::default(),
       http_client,
@@ -109,7 +108,7 @@ impl NpmRegistry {
     if matches!(self.inner().cache.cache_setting(), CacheSetting::Only) {
       return false;
     }
-    !self.inner().force_reload.swap(true, Ordering::SeqCst)
+    self.inner().force_reload_flag.raise()
   }
 
   fn inner(&self) -> &Arc<NpmRegistryApiInner> {
@@ -160,7 +159,7 @@ enum CacheItem {
 struct NpmRegistryApiInner {
   base_url: Url,
   cache: NpmCache,
-  force_reload: AtomicBool,
+  force_reload_flag: AtomicFlag,
   mem_cache: Mutex<HashMap<String, CacheItem>>,
   previously_reloaded_packages: Mutex<HashSet<String>>,
   http_client: HttpClient,
@@ -230,7 +229,7 @@ impl NpmRegistryApiInner {
   }
 
   fn force_reload(&self) -> bool {
-    self.force_reload.load(Ordering::SeqCst)
+    self.force_reload_flag.is_raised()
   }
 
   fn load_file_cached_package_info(

cli/proc_state.rs

@@ -393,6 +393,7 @@ impl ProcState {
 
     build_graph_with_npm_resolution(
       graph,
+      &self.resolver,
       &self.npm_resolver,
       roots.clone(),
       &mut cache,
@@ -695,6 +696,7 @@ impl ProcState {
     let mut graph = ModuleGraph::default();
     build_graph_with_npm_resolution(
       &mut graph,
+      &self.resolver,
       &self.npm_resolver,
       roots,
       loader,

cli/resolver.rs

@@ -24,6 +24,7 @@ use crate::args::JsxImportSourceConfig;
 use crate::npm::NpmRegistry;
 use crate::npm::NpmResolution;
 use crate::npm::PackageJsonDepsInstaller;
+use crate::util::sync::AtomicFlag;
 
 /// A resolver that takes care of resolution, taking into account loaded
 /// import map, JSX settings.
@@ -36,6 +37,7 @@ pub struct CliGraphResolver {
   npm_registry_api: NpmRegistry,
   npm_resolution: NpmResolution,
   package_json_deps_installer: PackageJsonDepsInstaller,
+  found_package_json_dep_flag: Arc<AtomicFlag>,
   sync_download_queue: Option<Arc<TaskQueue>>,
 }
 
@@ -54,6 +56,7 @@ impl Default for CliGraphResolver {
       npm_registry_api,
       npm_resolution,
       package_json_deps_installer: Default::default(),
+      found_package_json_dep_flag: Default::default(),
       sync_download_queue: Self::create_sync_download_queue(),
     }
   }
@@ -79,6 +82,7 @@ impl CliGraphResolver {
       npm_registry_api,
       npm_resolution,
       package_json_deps_installer,
+      found_package_json_dep_flag: Default::default(),
       sync_download_queue: Self::create_sync_download_queue(),
     }
   }
@@ -98,6 +102,18 @@ impl CliGraphResolver {
   pub fn as_graph_npm_resolver(&self) -> &dyn NpmResolver {
     self
   }
+
+  pub async fn top_level_package_json_install_if_necessary(
+    &self,
+  ) -> Result<(), AnyError> {
+    if self.found_package_json_dep_flag.is_raised() {
+      self
+        .package_json_deps_installer
+        .ensure_top_level_install()
+        .await?;
+    }
+    Ok(())
+  }
 }
 
 impl Resolver for CliGraphResolver {
@@ -132,6 +148,7 @@ impl Resolver for CliGraphResolver {
     if let Some(deps) = self.package_json_deps_installer.package_deps().as_ref()
     {
       if let Some(specifier) = resolve_package_json_dep(specifier, deps)? {
+        self.found_package_json_dep_flag.raise();
         return Ok(specifier);
       }
     }
@@ -195,7 +212,6 @@ impl NpmResolver for CliGraphResolver {
     // this will internally cache the package information
     let package_name = package_name.to_string();
     let api = self.npm_registry_api.clone();
-    let deps_installer = self.package_json_deps_installer.clone();
     let maybe_sync_download_queue = self.sync_download_queue.clone();
     async move {
       let permit = if let Some(task_queue) = &maybe_sync_download_queue {
@@ -204,21 +220,6 @@ impl NpmResolver for CliGraphResolver {
         None
       };
 
-      // trigger an npm install if the package name matches
-      // a package in the package.json
-      //
-      // todo(dsherret): ideally this would only download if a bare
-      // specifiy matched in the package.json, but deno_graph only
-      // calls this once per package name and we might resolve an
-      // npm specifier first which calls this, then a bare specifier
-      // second and that would cause this not to occur.
-      if deps_installer.has_package_name(&package_name) {
-        deps_installer
-          .ensure_top_level_install()
-          .await
-          .map_err(|err| format!("{err:#}"))?;
-      }
-
       let result = api
         .package_info(&package_name)
         .await

cli/tests/integration/cache_tests.rs

@@ -1,6 +1,7 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
 use test_util::env_vars_for_npm_tests;
+use test_util::TestContextBuilder;
 
 itest!(_036_import_map_fetch {
   args:
@@ -107,3 +108,22 @@ itest!(package_json_basic {
   copy_temp_dir: Some("package_json/basic"),
   exit_code: 0,
 });
+
+#[test]
+fn cache_matching_package_json_dep_should_not_install_all() {
+  let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+  let temp_dir = context.temp_dir();
+  temp_dir.write(
+    "package.json",
+    r#"{ "dependencies": { "@types/node": "18.8.2", "@denotest/esm-basic": "*" } }"#,
+  );
+  let output = context
+    .new_command()
+    .args("cache npm:@types/node@18.8.2")
+    .run();
+  output.assert_matches_text(concat!(
+    "Download http://localhost:4545/npm/registry/@types/node\n",
+    "Download http://localhost:4545/npm/registry/@types/node/node-18.8.2.tgz\n",
+    "Initialize @types/node@18.8.2\n",
+  ));
+}

cli/util/mod.rs

@@ -11,6 +11,7 @@ pub mod fs;
 pub mod logger;
 pub mod path;
 pub mod progress_bar;
+pub mod sync;
 pub mod text_encoding;
 pub mod time;
 pub mod unix;

cli/util/sync.rs

@@ -0,0 +1,35 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+use std::sync::atomic::AtomicBool;
+use std::sync::atomic::Ordering;
+
+/// Simplifies the use of an atomic boolean as a flag.
+#[derive(Debug, Default)]
+pub struct AtomicFlag(AtomicBool);
+
+impl AtomicFlag {
+  /// Raises the flag returning if the raise was successful.
+  pub fn raise(&self) -> bool {
+    !self.0.swap(true, Ordering::SeqCst)
+  }
+
+  /// Gets if the flag is raised.
+  pub fn is_raised(&self) -> bool {
+    self.0.load(Ordering::SeqCst)
+  }
+}
+
+#[cfg(test)]
+mod test {
+  use super::AtomicFlag;
+
+  #[test]
+  fn atomic_flag_raises() {
+    let flag = AtomicFlag::default();
+    assert!(!flag.is_raised()); // false by default
+    assert!(flag.raise());
+    assert!(flag.is_raised());
+    assert!(!flag.raise());
+    assert!(flag.is_raised());
+  }
+}