From 8820f6e922d3332d0fdd035b504897503d4cdf3a Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 11 Apr 2023 18:10:51 -0400 Subject: [PATCH] fix(npm): do not "npm install" when npm specifier happens to match package.json entry (#18660) --- cli/graph_util.rs | 8 +++++++ cli/npm/installer.rs | 23 ++++-------------- cli/npm/registry.rs | 11 ++++----- cli/proc_state.rs | 2 ++ cli/resolver.rs | 33 +++++++++++++------------- cli/tests/integration/cache_tests.rs | 20 ++++++++++++++++ cli/util/mod.rs | 1 + cli/util/sync.rs | 35 ++++++++++++++++++++++++++++ 8 files changed, 92 insertions(+), 41 deletions(-) create mode 100644 cli/util/sync.rs diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 8ea48702a2..df4e5132d2 100644 --- a/cli/graph_util.rs +++ b/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, 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?; diff --git a/cli/npm/installer.rs b/cli/npm/installer.rs index 6d048f7ca3..41d85a9b93 100644 --- a/cli/npm/installer.rs +++ b/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 } diff --git a/cli/npm/registry.rs b/cli/npm/registry.rs index b9bff09e65..a87365c9ca 100644 --- a/cli/npm/registry.rs +++ b/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 { @@ -160,7 +159,7 @@ enum CacheItem { struct NpmRegistryApiInner { base_url: Url, cache: NpmCache, - force_reload: AtomicBool, + force_reload_flag: AtomicFlag, mem_cache: Mutex>, previously_reloaded_packages: Mutex>, 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( diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 188f57289c..d3a0618ef6 100644 --- a/cli/proc_state.rs +++ b/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, diff --git a/cli/resolver.rs b/cli/resolver.rs index 46badfe8a4..56bae25c13 100644 --- a/cli/resolver.rs +++ b/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, sync_download_queue: Option>, } @@ -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 diff --git a/cli/tests/integration/cache_tests.rs b/cli/tests/integration/cache_tests.rs index c80f7f5c85..32508a95ad 100644 --- a/cli/tests/integration/cache_tests.rs +++ b/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", + )); +} diff --git a/cli/util/mod.rs b/cli/util/mod.rs index c3133cc105..61511679fc 100644 --- a/cli/util/mod.rs +++ b/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; diff --git a/cli/util/sync.rs b/cli/util/sync.rs new file mode 100644 index 0000000000..6d136abc1f --- /dev/null +++ b/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()); + } +}