From fb04e87387e04053bf41a1512b4850adf62202c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 27 Nov 2022 13:25:08 -0500 Subject: [PATCH] fix(npm): ensure npm package downloaded once per run when using `--reload` (#16842) --- cli/lsp/language_server.rs | 12 +++---- cli/npm/cache.rs | 34 +++++++++++++++---- cli/npm/registry.rs | 23 +++++++------ cli/npm/resolvers/local.rs | 4 ++- cli/proc_state.rs | 1 - cli/tests/npm_tests.rs | 7 ++++ .../main.out | 5 +++ .../main.ts | 7 ++++ .../other.ts | 3 ++ 9 files changed, 71 insertions(+), 25 deletions(-) create mode 100644 cli/tests/testdata/npm/dynamic_import_reload_same_package/main.out create mode 100644 cli/tests/testdata/npm/dynamic_import_reload_same_package/main.ts create mode 100644 cli/tests/testdata/npm/dynamic_import_reload_same_package/other.ts diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 4e7c4b240b..081bbf4296 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -238,22 +238,20 @@ fn create_lsp_npm_resolver( http_client: HttpClient, ) -> NpmPackageResolver { let registry_url = RealNpmRegistryApi::default_url(); - // Use an "only" cache setting in order to make the - // user do an explicit "cache" command and prevent - // the cache from being filled with lots of packages while - // the user is typing. - let cache_setting = CacheSetting::Only; let progress_bar = ProgressBar::default(); let npm_cache = NpmCache::from_deno_dir( dir, - cache_setting.clone(), + // Use an "only" cache setting in order to make the + // user do an explicit "cache" command and prevent + // the cache from being filled with lots of packages while + // the user is typing. + CacheSetting::Only, http_client.clone(), progress_bar.clone(), ); let api = RealNpmRegistryApi::new( registry_url, npm_cache.clone(), - cache_setting, http_client, progress_bar, ); diff --git a/cli/npm/cache.rs b/cli/npm/cache.rs index b2c8423092..5e2f06ef74 100644 --- a/cli/npm/cache.rs +++ b/cli/npm/cache.rs @@ -1,14 +1,17 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. +use std::collections::HashSet; use std::fs; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; use deno_ast::ModuleSpecifier; use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::custom_error; use deno_core::error::AnyError; +use deno_core::parking_lot::Mutex; use deno_core::url::Url; use crate::cache::DenoDir; @@ -317,6 +320,8 @@ pub struct NpmCache { cache_setting: CacheSetting, http_client: HttpClient, progress_bar: ProgressBar, + /// ensures a package is only downloaded once per run + previously_reloaded_packages: Arc>>, } impl NpmCache { @@ -331,6 +336,7 @@ impl NpmCache { cache_setting, http_client, progress_bar, + previously_reloaded_packages: Default::default(), } } @@ -338,6 +344,26 @@ impl NpmCache { self.readonly.clone() } + pub fn cache_setting(&self) -> &CacheSetting { + &self.cache_setting + } + + /// Checks if the cache should be used for the provided name and version. + /// NOTE: Subsequent calls for the same package will always return `true` + /// to ensure a package is only downloaded once per run of the CLI. This + /// prevents downloads from re-occurring when someone has `--reload` and + /// and imports a dynamic import that imports the same package again for example. + fn should_use_global_cache_for_package( + &self, + package: (&str, &NpmVersion), + ) -> bool { + self.cache_setting.should_use_for_npm_package(package.0) + || !self + .previously_reloaded_packages + .lock() + .insert(format!("{}@{}", package.0, package.1)) + } + pub async fn ensure_package( &self, package: (&str, &NpmVersion), @@ -352,10 +378,6 @@ impl NpmCache { }) } - pub fn should_use_cache_for_npm_package(&self, package_name: &str) -> bool { - self.cache_setting.should_use_for_npm_package(package_name) - } - async fn ensure_package_inner( &self, package: (&str, &NpmVersion), @@ -367,11 +389,11 @@ impl NpmCache { package.1, registry_url, ); - if package_folder.exists() + if self.should_use_global_cache_for_package(package) + && package_folder.exists() // if this file exists, then the package didn't successfully extract // the first time, or another process is currently extracting the zip file && !package_folder.join(NPM_PACKAGE_SYNC_LOCK_FILENAME).exists() - && self.should_use_cache_for_npm_package(package.0) { return Ok(()); } else if self.cache_setting == CacheSetting::Only { diff --git a/cli/npm/registry.rs b/cli/npm/registry.rs index 3f0a841655..c62e6e1e7d 100644 --- a/cli/npm/registry.rs +++ b/cli/npm/registry.rs @@ -2,6 +2,7 @@ use std::cmp::Ordering; use std::collections::HashMap; +use std::collections::HashSet; use std::fs; use std::io::ErrorKind; use std::path::PathBuf; @@ -248,7 +249,6 @@ impl RealNpmRegistryApi { pub fn new( base_url: Url, cache: NpmCache, - cache_setting: CacheSetting, http_client: HttpClient, progress_bar: ProgressBar, ) -> Self { @@ -256,7 +256,7 @@ impl RealNpmRegistryApi { base_url, cache, mem_cache: Default::default(), - cache_setting, + previously_reloaded_packages: Default::default(), http_client, progress_bar, })) @@ -286,7 +286,7 @@ struct RealNpmRegistryApiInner { base_url: Url, cache: NpmCache, mem_cache: Mutex>>>, - cache_setting: CacheSetting, + previously_reloaded_packages: Mutex>, http_client: HttpClient, progress_bar: ProgressBar, } @@ -296,12 +296,16 @@ impl RealNpmRegistryApiInner { &self, name: &str, ) -> Result>, AnyError> { - let maybe_info = self.mem_cache.lock().get(name).cloned(); - if let Some(info) = maybe_info { - Ok(info) + let maybe_maybe_info = self.mem_cache.lock().get(name).cloned(); + if let Some(maybe_info) = maybe_maybe_info { + Ok(maybe_info) } else { let mut maybe_package_info = None; - if self.cache_setting.should_use_for_npm_package(name) { + if self.cache.cache_setting().should_use_for_npm_package(name) + // if this has been previously reloaded, then try loading from the + // file system cache + || !self.previously_reloaded_packages.lock().insert(name.to_string()) + { // attempt to load from the file cache maybe_package_info = self.load_file_cached_package_info(name); } @@ -409,15 +413,14 @@ impl RealNpmRegistryApiInner { &self, name: &str, ) -> Result, AnyError> { - if self.cache_setting == CacheSetting::Only { + if *self.cache.cache_setting() == CacheSetting::Only { return Err(custom_error( "NotCached", format!( "An npm specifier not found in cache: \"{}\", --cached-only is specified.", name ) - ) - ); + )); } let package_url = self.get_package_url(name); diff --git a/cli/npm/resolvers/local.rs b/cli/npm/resolvers/local.rs index a6df641d19..ff699f26f3 100644 --- a/cli/npm/resolvers/local.rs +++ b/cli/npm/resolvers/local.rs @@ -291,7 +291,9 @@ async fn sync_resolution_with_fs( get_package_folder_id_folder_name(&package.get_package_cache_folder_id()); let folder_path = deno_local_registry_dir.join(&folder_name); let initialized_file = folder_path.join(".initialized"); - if !cache.should_use_cache_for_npm_package(&package.id.name) + if !cache + .cache_setting() + .should_use_for_npm_package(&package.id.name) || !initialized_file.exists() { let cache = cache.clone(); diff --git a/cli/proc_state.rs b/cli/proc_state.rs index dc80ca8db7..3b7a975735 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -225,7 +225,6 @@ impl ProcState { let api = RealNpmRegistryApi::new( registry_url, npm_cache.clone(), - cli_options.cache_setting(), http_client, progress_bar.clone(), ); diff --git a/cli/tests/npm_tests.rs b/cli/tests/npm_tests.rs index 99e4756208..3939ee7bec 100644 --- a/cli/tests/npm_tests.rs +++ b/cli/tests/npm_tests.rs @@ -155,6 +155,13 @@ mod npm { // http_server: true, // }); + itest!(dynamic_import_reload_same_package { + args: "run -A --reload npm/dynamic_import_reload_same_package/main.ts", + output: "npm/dynamic_import_reload_same_package/main.out", + envs: env_vars(), + http_server: true, + }); + itest!(env_var_re_export_dev { args: "run --allow-read --allow-env --quiet npm/env_var_re_export/main.js", output_str: Some("dev\n"), diff --git a/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.out b/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.out new file mode 100644 index 0000000000..918e7f5e8f --- /dev/null +++ b/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.out @@ -0,0 +1,5 @@ +Download http://localhost:4545/npm/registry/chalk +Download http://localhost:4545/npm/registry/chalk/chalk-5.0.1.tgz +Starting... +Ran other. +Finished... diff --git a/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.ts b/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.ts new file mode 100644 index 0000000000..7c7ee7d557 --- /dev/null +++ b/cli/tests/testdata/npm/dynamic_import_reload_same_package/main.ts @@ -0,0 +1,7 @@ +import chalk from "npm:chalk@5"; + +console.log(chalk.green("Starting...")); +// non-analyzable +const importName = "./other.ts"; +await import(importName); +console.log(chalk.green("Finished...")); diff --git a/cli/tests/testdata/npm/dynamic_import_reload_same_package/other.ts b/cli/tests/testdata/npm/dynamic_import_reload_same_package/other.ts new file mode 100644 index 0000000000..28e3da14ff --- /dev/null +++ b/cli/tests/testdata/npm/dynamic_import_reload_same_package/other.ts @@ -0,0 +1,3 @@ +import chalk from "npm:chalk@5"; + +console.log(chalk.green("Ran other."));