fix(npm): ensure npm package downloaded once per run when using `--reload` (#16842)

9 files changed, 71 insertions, 25 deletions

diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
index 4e7c4b240..081bbf429 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 b2c842309..5e2f06ef7 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<Mutex<HashSet<String>>>,
 }
 
 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 3f0a84165..c62e6e1e7 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<HashMap<String, Option<Arc<NpmPackageInfo>>>>,
-  cache_setting: CacheSetting,
+  previously_reloaded_packages: Mutex<HashSet<String>>,
   http_client: HttpClient,
   progress_bar: ProgressBar,
 }
@@ -296,12 +296,16 @@ impl RealNpmRegistryApiInner {
     &self,
     name: &str,
   ) -> Result<Option<Arc<NpmPackageInfo>>, 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<Option<NpmPackageInfo>, 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 a6df641d1..ff699f26f 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 dc80ca8db..3b7a97573 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 99e475620..3939ee7be 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 000000000..918e7f5e8
--- /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 000000000..7c7ee7d55
--- /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 000000000..28e3da14f
--- /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."));