fix(npm): do not "npm install" when npm specifier happens to match pa…

…ckage.json entry

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,13 +250,20 @@ 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,
  options: deno_graph::BuildOptions<'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::synchronization::AtomicFlag;
 
 use super::CliNpmRegistryApi;
 use super::NpmResolution;
 
 #[derive(Debug)]
 struct PackageJsonDepsInstallerInner {
- has_installed: AtomicBool,
+ has_installed_flag: AtomicFlag,
  npm_registry_api: CliNpmRegistryApi,
  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::synchronization::AtomicFlag;
 
 use super::cache::should_sync_download;
 use super::cache::NpmCache;
@@ -70,7 +69,7 @@ impl CliNpmRegistryApi {
  Self(Some(Arc::new(CliNpmRegistryApiInner {
  base_url,
  cache,
- force_reload: AtomicBool::new(false),
+ force_reload_flag: Default::default(),
  mem_cache: Default::default(),
  previously_reloaded_packages: Default::default(),
  http_client,
@@ -113,7 +112,7 @@ impl CliNpmRegistryApi {
  ) {
  return false;
  }
- !self.inner().force_reload.swap(true, Ordering::SeqCst)
+ self.inner().force_reload_flag.raise()
  }
 
  fn inner(&self) -> &Arc<CliNpmRegistryApiInner> {
@@ -165,7 +164,7 @@ enum CacheItem {
 struct CliNpmRegistryApiInner {
  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,
@@ -241,7 +240,7 @@ impl CliNpmRegistryApiInner {
  }
 
  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

@@ -23,6 +23,7 @@ use crate::args::JsxImportSourceConfig;
 use crate::npm::CliNpmRegistryApi;
 use crate::npm::NpmResolution;
 use crate::npm::PackageJsonDepsInstaller;
+use crate::util::synchronization::AtomicFlag;
 
 /// A resolver that takes care of resolution, taking into account loaded
 /// import map, JSX settings.
@@ -35,6 +36,7 @@ pub struct CliGraphResolver {
  npm_registry_api: CliNpmRegistryApi,
  npm_resolution: NpmResolution,
  package_json_deps_installer: PackageJsonDepsInstaller,
+ found_package_json_dep_flag: Arc<AtomicFlag>,
  sync_download_queue: Option<Arc<TaskQueue>>,
 }
 
@@ -53,6 +55,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(),
  }
  }
@@ -78,6 +81,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(),
  }
  }
@@ -97,6 +101,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 {
@@ -131,6 +147,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);
  }
  }
@@ -194,7 +211,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 {
@@ -203,20 +219,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. We also need
- // to know if the bare specifier is from a package.json and not
- // from an import map
- if deps_installer.has_package_name(&package_name) {
- deps_installer.ensure_top_level_install().await?;
- }
-
  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,19 @@ itest!(package_json_basic {
  copy_temp_dir: Some("package_json/basic"),
  exit_code: 0,
 });
+
+#[test]
+fn cache_matching_package_json_dep_should_not_reload() {
+ let context = TestContextBuilder::for_npm().use_temp_cwd().build();
+ let temp_dir = context.temp_dir();
+ temp_dir.write(
+ "package.json",
+ r#"{ "dependencies": { "@types/node": "*", "@denotest/esm-basic": "*" } }"#,
+ );
+ let output = context.new_command().args("cache npm:@types/node").run();
+ output.assert_matches_text(concat!(
+ "Download http:https://localhost:4545/npm/registry/@types/node\n",
+ "Download http:https://localhost:4545/npm/registry/@types/node/node-18.8.2.tgz\n",
+ "Initialize @types/[email protected]\n",
+ ));
+}

cli/util/mod.rs

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

cli/util/synchronization.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());
+ }
+}