fix(npm): cache bust npm specifiers more aggressively (#18636)

Part 1: #18622 Part 2: This PR Closes #16901 --------- Co-authored-by: Luca Casonato <[email protected]>
denoland · Apr 12, 2023 · 4fab252 · 4fab252
1 parent b183737
commit 4fab252
Show file tree

Hide file tree

Showing 14 changed files with 291 additions and 145 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cli/Cargo.toml b/cli/Cargo.toml
@@ -42,9 +42,9 @@ winres.workspace = true
 [dependencies]
 deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_graph", "module_specifier", "proposal", "react", "sourcemap", "transforms", "typescript", "view", "visit"] }
 deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] }
-deno_doc = "0.60.0"
-deno_emit = "0.18.0"
-deno_graph = "=0.46.0"
+deno_doc = "0.61.0"
+deno_emit = "0.19.0"
+deno_graph = "=0.47.1"
 deno_lint = { version = "0.43.0", features = ["docs"] }
 deno_lockfile.workspace = true
 deno_npm = "0.2.0"
@@ -70,7 +70,7 @@ dprint-plugin-markdown = "=0.15.2"
 dprint-plugin-typescript = "=0.84.0"
 encoding_rs.workspace = true
 env_logger = "=0.9.0"
-eszip = "=0.39.0"
+eszip = "=0.40.0"
 fancy-regex = "=0.10.0"
 flate2.workspace = true
 fs3.workspace = true

diff --git a/cli/args/mod.rs b/cli/args/mod.rs
@@ -64,8 +64,8 @@ use std::sync::Arc;
 
 use crate::cache::DenoDir;
 use crate::file_fetcher::FileFetcher;
+use crate::npm::CliNpmRegistryApi;
 use crate::npm::NpmProcessState;
-use crate::npm::NpmRegistry;
 use crate::util::fs::canonicalize_path_maybe_not_exists;
 use crate::version;
 
@@ -744,7 +744,7 @@ impl CliOptions {
 
   pub async fn resolve_npm_resolution_snapshot(
     &self,
-    api: &NpmRegistry,
+    api: &CliNpmRegistryApi,
   ) -> Result<Option<NpmResolutionSnapshot>, AnyError> {
     if let Some(state) = &*NPM_PROCESS_STATE {
       // TODO(bartlomieju): remove this clone

diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs
@@ -20,8 +20,8 @@ use crate::lsp::logging::lsp_warn;
 use crate::node;
 use crate::node::node_resolve_npm_reference;
 use crate::node::NodeResolution;
+use crate::npm::CliNpmRegistryApi;
 use crate::npm::NpmPackageResolver;
-use crate::npm::NpmRegistry;
 use crate::npm::NpmResolution;
 use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
@@ -1166,7 +1166,7 @@ impl Documents {
     maybe_import_map: Option<Arc<import_map::ImportMap>>,
     maybe_config_file: Option<&ConfigFile>,
     maybe_package_json: Option<&PackageJson>,
-    npm_registry_api: NpmRegistry,
+    npm_registry_api: CliNpmRegistryApi,
     npm_resolution: NpmResolution,
   ) {
     fn calculate_resolver_config_hash(
@@ -1864,7 +1864,7 @@ console.log(b, "hello deno");
 
   #[test]
   fn test_documents_refresh_dependencies_config_change() {
-    let npm_registry_api = NpmRegistry::new_uninitialized();
+    let npm_registry_api = CliNpmRegistryApi::new_uninitialized();
     let npm_resolution =
       NpmResolution::new(npm_registry_api.clone(), None, None);
 

diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs
@@ -79,9 +79,9 @@ use crate::graph_util;
 use crate::http_util::HttpClient;
 use crate::lsp::urls::LspUrlKind;
 use crate::npm::create_npm_fs_resolver;
+use crate::npm::CliNpmRegistryApi;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageResolver;
-use crate::npm::NpmRegistry;
 use crate::npm::NpmResolution;
 use crate::proc_state::ProcState;
 use crate::tools::fmt::format_file;
@@ -145,7 +145,7 @@ pub struct Inner {
   /// A lazily create "server" for handling test run requests.
   maybe_testing_server: Option<testing::TestServer>,
   /// Npm's registry api.
-  npm_api: NpmRegistry,
+  npm_api: CliNpmRegistryApi,
   /// Npm cache
   npm_cache: NpmCache,
   /// Npm resolution that is stored in memory.
@@ -417,8 +417,13 @@ impl LanguageServer {
 fn create_lsp_structs(
   dir: &DenoDir,
   http_client: HttpClient,
-) -> (NpmRegistry, NpmCache, NpmPackageResolver, NpmResolution) {
-  let registry_url = NpmRegistry::default_url();
+) -> (
+  CliNpmRegistryApi,
+  NpmCache,
+  NpmPackageResolver,
+  NpmResolution,
+) {
+  let registry_url = CliNpmRegistryApi::default_url();
   let progress_bar = ProgressBar::new(ProgressBarStyle::TextOnly);
   let npm_cache = NpmCache::from_deno_dir(
     dir,
@@ -430,7 +435,7 @@ fn create_lsp_structs(
     http_client.clone(),
     progress_bar.clone(),
   );
-  let api = NpmRegistry::new(
+  let api = CliNpmRegistryApi::new(
     registry_url.clone(),
     npm_cache.clone(),
     http_client,

diff --git a/cli/npm/installer.rs b/cli/npm/installer.rs
@@ -1,33 +1,69 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use std::future::Future;
 use std::sync::Arc;
 
 use deno_core::error::AnyError;
 use deno_core::futures::stream::FuturesOrdered;
 use deno_core::futures::StreamExt;
 use deno_npm::registry::NpmRegistryApi;
+use deno_npm::registry::NpmRegistryPackageInfoLoadError;
+use deno_semver::npm::NpmPackageReq;
 
 use crate::args::package_json::PackageJsonDeps;
 use crate::util::sync::AtomicFlag;
 
-use super::NpmRegistry;
+use super::CliNpmRegistryApi;
 use super::NpmResolution;
 
 #[derive(Debug)]
 struct PackageJsonDepsInstallerInner {
   has_installed_flag: AtomicFlag,
-  npm_registry_api: NpmRegistry,
+  npm_registry_api: CliNpmRegistryApi,
   npm_resolution: NpmResolution,
   package_deps: PackageJsonDeps,
 }
 
+impl PackageJsonDepsInstallerInner {
+  pub fn reqs(&self) -> Vec<&NpmPackageReq> {
+    let mut package_reqs = self
+      .package_deps
+      .values()
+      .filter_map(|r| r.as_ref().ok())
+      .collect::<Vec<_>>();
+    package_reqs.sort(); // deterministic resolution
+    package_reqs
+  }
+
+  pub fn reqs_with_info_futures(
+    &self,
+  ) -> FuturesOrdered<
+    impl Future<
+      Output = Result<
+        (&NpmPackageReq, Arc<deno_npm::registry::NpmPackageInfo>),
+        NpmRegistryPackageInfoLoadError,
+      >,
+    >,
+  > {
+    let package_reqs = self.reqs();
+
+    FuturesOrdered::from_iter(package_reqs.into_iter().map(|req| {
+      let api = self.npm_registry_api.clone();
+      async move {
+        let info = api.package_info(&req.name).await?;
+        Ok::<_, NpmRegistryPackageInfoLoadError>((req, info))
+      }
+    }))
+  }
+}
+
 /// Holds and controls installing dependencies from package.json.
 #[derive(Debug, Clone, Default)]
 pub struct PackageJsonDepsInstaller(Option<Arc<PackageJsonDepsInstallerInner>>);
 
 impl PackageJsonDepsInstaller {
   pub fn new(
-    npm_registry_api: NpmRegistry,
+    npm_registry_api: CliNpmRegistryApi,
     npm_resolution: NpmResolution,
     deps: Option<PackageJsonDeps>,
   ) -> Self {
@@ -57,27 +93,23 @@ impl PackageJsonDepsInstaller {
       return Ok(()); // already installed by something else
     }
 
-    let mut package_reqs = inner
-      .package_deps
-      .values()
-      .filter_map(|r| r.as_ref().ok())
-      .collect::<Vec<_>>();
-    package_reqs.sort(); // deterministic resolution
-
-    let mut req_with_infos =
-      FuturesOrdered::from_iter(package_reqs.into_iter().map(|req| {
-        let api = inner.npm_registry_api.clone();
-        async move {
-          let info = api.package_info(&req.name).await?;
-          Ok::<_, AnyError>((req, info))
-        }
-      }));
+    let mut reqs_with_info_futures = inner.reqs_with_info_futures();
 
-    while let Some(result) = req_with_infos.next().await {
+    while let Some(result) = reqs_with_info_futures.next().await {
       let (req, info) = result?;
-      inner
+      let result = inner
         .npm_resolution
-        .resolve_package_req_as_pending_with_info(req, &info)?;
+        .resolve_package_req_as_pending_with_info(req, &info);
+      if let Err(err) = result {
+        if inner.npm_registry_api.mark_force_reload() {
+          log::debug!("Failed to resolve package. Retrying. Error: {err:#}");
+          // re-initialize
+          inner.npm_registry_api.clear_memory_cache();
+          reqs_with_info_futures = inner.reqs_with_info_futures();
+        } else {
+          return Err(err.into());
+        }
+      }
     }
 
     Ok(())

diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs
@@ -10,7 +10,7 @@ mod tarball;
 pub use cache::should_sync_download;
 pub use cache::NpmCache;
 pub use installer::PackageJsonDepsInstaller;
-pub use registry::NpmRegistry;
+pub use registry::CliNpmRegistryApi;
 pub use resolution::NpmResolution;
 pub use resolvers::create_npm_fs_resolver;
 pub use resolvers::NpmPackageResolver;