refactor(npm): push npm struct creation to a higher level (#18139)

This has been bothering me for a while and it became more painful while
working on #18136 because injecting the shared progress bar became very
verbose. Basically we should move the creation of all these npm structs
up to a higher level.

This is a stepping stone for a future refactor where we can improve how
we create all our structs.

cli/args/mod.rs

@@ -12,6 +12,7 @@ use self::package_json::PackageJsonDeps;
 use ::import_map::ImportMap;
 use indexmap::IndexMap;
 
+use crate::npm::NpmRegistryApi;
 use crate::npm::NpmResolutionSnapshot;
 pub use config_file::BenchConfig;
 pub use config_file::CompilerOptions;
@@ -664,13 +665,31 @@ impl CliOptions {
     .map(Some)
   }
 
-  pub fn get_npm_resolution_snapshot(&self) -> Option<NpmResolutionSnapshot> {
+  pub async fn resolve_npm_resolution_snapshot(
+    &self,
+    api: &NpmRegistryApi,
+  ) -> Result<Option<NpmResolutionSnapshot>, AnyError> {
     if let Some(state) = &*NPM_PROCESS_STATE {
       // TODO(bartlomieju): remove this clone
-      return Some(state.snapshot.clone());
+      return Ok(Some(state.snapshot.clone()));
     }
 
-    None
+    if let Some(lockfile) = self.maybe_lock_file() {
+      if !lockfile.lock().overwrite {
+        return Ok(Some(
+          NpmResolutionSnapshot::from_lockfile(lockfile.clone(), api)
+            .await
+            .with_context(|| {
+              format!(
+                "failed reading lockfile '{}'",
+                lockfile.lock().filename.display()
+              )
+            })?,
+        ));
+      }
+    }
+
+    Ok(None)
   }
 
   // If the main module should be treated as being in an npm package.

cli/graph_util.rs

@@ -165,8 +165,8 @@ pub async fn create_graph_and_maybe_check(
     ps.options.to_maybe_jsx_import_source_config(),
     ps.maybe_import_map.clone(),
     ps.options.no_npm(),
-    ps.npm_resolver.api().clone(),
-    ps.npm_resolver.resolution().clone(),
+    ps.npm_api.clone(),
+    ps.npm_resolution.clone(),
     ps.package_json_deps_installer.clone(),
   );
   let graph_resolver = cli_resolver.as_graph_resolver();

cli/lsp/diagnostics.rs

@@ -911,7 +911,6 @@ fn diagnose_resolution(
         if let Some(npm_resolver) = &snapshot.maybe_npm_resolver {
           // show diagnostics for npm package references that aren't cached
           if npm_resolver
-            .resolution()
             .resolve_pkg_id_from_pkg_req(&pkg_ref.req)
             .is_err()
           {
@@ -933,7 +932,6 @@ fn diagnose_resolution(
           let types_node_ref =
             NpmPackageReqReference::from_str("npm:@types/node").unwrap();
           if npm_resolver
-            .resolution()
             .resolve_pkg_id_from_pkg_req(&types_node_ref.req)
             .is_err()
           {

cli/lsp/documents.rs

@@ -1409,7 +1409,6 @@ fn node_resolve_npm_req_ref(
   maybe_npm_resolver.map(|npm_resolver| {
     NodeResolution::into_specifier_and_media_type(
       npm_resolver
-        .resolution()
         .pkg_req_ref_to_nv_ref(npm_req_ref)
         .ok()
         .and_then(|pkg_id_ref| {

cli/lsp/language_server.rs

@@ -75,9 +75,11 @@ use crate::cache::HttpCache;
 use crate::file_fetcher::FileFetcher;
 use crate::graph_util;
 use crate::http_util::HttpClient;
+use crate::npm::create_npm_fs_resolver;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageResolver;
 use crate::npm::NpmRegistryApi;
+use crate::npm::NpmResolution;
 use crate::proc_state::ProcState;
 use crate::tools::fmt::format_file;
 use crate::tools::fmt::format_parsed_source;
@@ -140,6 +142,12 @@ pub struct Inner {
   lint_options: LintOptions,
   /// A lazily create "server" for handling test run requests.
   maybe_testing_server: Option<testing::TestServer>,
+  /// Npm's registry api.
+  npm_api: NpmRegistryApi,
+  /// Npm cache
+  npm_cache: NpmCache,
+  /// Npm resolution that is stored in memory.
+  npm_resolution: NpmResolution,
   /// Resolver for npm packages.
   npm_resolver: NpmPackageResolver,
   /// A collection of measurements which instrument that performance of the LSP.
@@ -317,10 +325,10 @@ impl LanguageServer {
   }
 }
 
-fn create_lsp_npm_resolver(
+fn create_lsp_structs(
   dir: &DenoDir,
   http_client: HttpClient,
-) -> NpmPackageResolver {
+) -> (NpmRegistryApi, NpmCache, NpmPackageResolver, NpmResolution) {
   let registry_url = NpmRegistryApi::default_url();
   let progress_bar = ProgressBar::new(ProgressBarStyle::TextOnly);
   let npm_cache = NpmCache::from_deno_dir(
@@ -339,7 +347,19 @@ fn create_lsp_npm_resolver(
     http_client,
     progress_bar,
   );
-  NpmPackageResolver::new(npm_cache, api)
+  let resolution = NpmResolution::new(api.clone(), None, None);
+  let fs_resolver = create_npm_fs_resolver(
+    npm_cache.clone(),
+    registry_url.clone(),
+    resolution.clone(),
+    None,
+  );
+  (
+    api,
+    npm_cache,
+    NpmPackageResolver::new(resolution.clone(), fs_resolver, None),
+    resolution,
+  )
 }
 
 impl Inner {
@@ -365,7 +385,8 @@ impl Inner {
       ts_server.clone(),
     );
     let assets = Assets::new(ts_server.clone());
-    let npm_resolver = create_lsp_npm_resolver(&dir, http_client.clone());
+    let (npm_api, npm_cache, npm_resolver, npm_resolution) =
+      create_lsp_structs(&dir, http_client.clone());
 
     Self {
       assets,
@@ -386,6 +407,9 @@ impl Inner {
       maybe_testing_server: None,
       module_registries,
       module_registries_location,
+      npm_api,
+      npm_cache,
+      npm_resolution,
       npm_resolver,
       performance,
       ts_fixable_diagnostics: Default::default(),
@@ -574,7 +598,24 @@ impl Inner {
       cache_metadata: self.cache_metadata.clone(),
       documents: self.documents.clone(),
       maybe_import_map: self.maybe_import_map.clone(),
-      maybe_npm_resolver: Some(self.npm_resolver.snapshotted()),
+      maybe_npm_resolver: Some({
+        // create a new snapshotted npm resolution and resolver
+        let resolution = NpmResolution::new(
+          self.npm_api.clone(),
+          Some(self.npm_resolution.snapshot()),
+          None,
+        );
+        NpmPackageResolver::new(
+          resolution.clone(),
+          create_npm_fs_resolver(
+            self.npm_cache.clone(),
+            self.npm_api.base_url().clone(),
+            resolution,
+            None,
+          ),
+          None,
+        )
+      }),
     })
   }
 
@@ -643,7 +684,12 @@ impl Inner {
       self.http_client.clone(),
     )?;
     self.module_registries_location = module_registries_location;
-    self.npm_resolver = create_lsp_npm_resolver(&dir, self.http_client.clone());
+    (
+      self.npm_api,
+      self.npm_cache,
+      self.npm_resolver,
+      self.npm_resolution,
+    ) = create_lsp_structs(&dir, self.http_client.clone());
     // update the cache path
     let location = dir.deps_folder_path();
     self.documents.set_location(&location);
@@ -987,8 +1033,8 @@ impl Inner {
       self.maybe_import_map.clone(),
       self.maybe_config_file.as_ref(),
       self.maybe_package_json.as_ref(),
-      self.npm_resolver.api().clone(),
-      self.npm_resolver.resolution().clone(),
+      self.npm_api.clone(),
+      self.npm_resolution.clone(),
     );
 
     self.assets.intitialize(self.snapshot()).await;
@@ -1180,8 +1226,8 @@ impl Inner {
       self.maybe_import_map.clone(),
       self.maybe_config_file.as_ref(),
       self.maybe_package_json.as_ref(),
-      self.npm_resolver.api().clone(),
-      self.npm_resolver.resolution().clone(),
+      self.npm_api.clone(),
+      self.npm_resolution.clone(),
     );
 
     self.send_diagnostics_update();
@@ -1238,8 +1284,8 @@ impl Inner {
         self.maybe_import_map.clone(),
         self.maybe_config_file.as_ref(),
         self.maybe_package_json.as_ref(),
-        self.npm_resolver.api().clone(),
-        self.npm_resolver.resolution().clone(),
+        self.npm_api.clone(),
+        self.npm_resolution.clone(),
       );
       self.refresh_npm_specifiers().await;
       self.diagnostics_server.invalidate_all();

cli/npm/mod.rs

@@ -19,5 +19,6 @@ pub use resolution::NpmPackageId;
 pub use resolution::NpmResolution;
 pub use resolution::NpmResolutionPackage;
 pub use resolution::NpmResolutionSnapshot;
+pub use resolvers::create_npm_fs_resolver;
 pub use resolvers::NpmPackageResolver;
 pub use resolvers::NpmProcessState;

cli/npm/resolution/mod.rs

@@ -236,6 +236,9 @@ impl NpmResolutionPackage {
   }
 }
 
+/// Handles updating and storing npm resolution in memory.
+///
+/// This does not interact with the file system.
 #[derive(Clone)]
 pub struct NpmResolution(Arc<NpmResolutionInner>);
 

cli/npm/resolvers/common.rs

@@ -23,6 +23,9 @@ pub trait NpmPackageFsResolver: Send + Sync {
   /// Specifier for the root directory.
   fn root_dir_url(&self) -> &Url;
 
+  /// The local node_modules folder if it is applicable to the implementation.
+  fn node_modules_path(&self) -> Option<PathBuf>;
+
   fn resolve_package_folder_from_deno_module(
     &self,
     id: &NpmPackageId,

cli/npm/resolvers/global.rs

@@ -72,6 +72,10 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver {
     self.cache.root_dir_url()
   }
 
+  fn node_modules_path(&self) -> Option<PathBuf> {
+    None
+  }
+
   fn resolve_package_folder_from_deno_module(
     &self,
     id: &NpmPackageId,

cli/npm/resolvers/local.rs

@@ -128,6 +128,10 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
     &self.root_node_modules_url
   }
 
+  fn node_modules_path(&self) -> Option<PathBuf> {
+    Some(self.root_node_modules_path.clone())
+  }
+
   fn resolve_package_folder_from_deno_module(
     &self,
     node_id: &NpmPackageId,

cli/npm/resolvers/mod.rs

@@ -6,12 +6,14 @@ mod local;
 
 use deno_ast::ModuleSpecifier;
 use deno_core::anyhow::bail;
-use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
 use deno_core::parking_lot::Mutex;
 use deno_core::serde_json;
+use deno_core::url::Url;
 use deno_graph::npm::NpmPackageNv;
+use deno_graph::npm::NpmPackageNvReference;
 use deno_graph::npm::NpmPackageReq;
+use deno_graph::npm::NpmPackageReqReference;
 use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::PathClean;
@@ -31,7 +33,6 @@ use self::local::LocalNpmPackageResolver;
 use super::resolution::NpmResolution;
 use super::NpmCache;
 use super::NpmPackageId;
-use super::NpmRegistryApi;
 use super::NpmResolutionSnapshot;
 
 /// State provided to the process via an environment variable.
@@ -41,13 +42,11 @@ pub struct NpmProcessState {
   pub local_node_modules_path: Option<String>,
 }
 
+/// Brings together the npm resolution with the file system.
 #[derive(Clone)]
 pub struct NpmPackageResolver {
   fs_resolver: Arc<dyn NpmPackageFsResolver>,
-  local_node_modules_path: Option<PathBuf>,
-  api: NpmRegistryApi,
   resolution: NpmResolution,
-  cache: NpmCache,
   maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
 }
 
@@ -55,95 +54,37 @@ impl std::fmt::Debug for NpmPackageResolver {
   fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
     f.debug_struct("NpmPackageResolver")
       .field("fs_resolver", &"<omitted>")
-      .field("local_node_modules_path", &self.local_node_modules_path)
-      .field("api", &"<omitted>")
       .field("resolution", &"<omitted>")
-      .field("cache", &"<omitted>")
       .field("maybe_lockfile", &"<omitted>")
       .finish()
   }
 }
 
 impl NpmPackageResolver {
-  pub fn new(cache: NpmCache, api: NpmRegistryApi) -> Self {
-    Self::new_inner(cache, api, None, None, None)
-  }
-
-  pub async fn new_with_maybe_lockfile(
-    cache: NpmCache,
-    api: NpmRegistryApi,
-    local_node_modules_path: Option<PathBuf>,
-    initial_snapshot: Option<NpmResolutionSnapshot>,
-    maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
-  ) -> Result<Self, AnyError> {
-    let mut initial_snapshot = initial_snapshot;
-
-    if initial_snapshot.is_none() {
-      if let Some(lockfile) = &maybe_lockfile {
-        if !lockfile.lock().overwrite {
-          initial_snapshot = Some(
-            NpmResolutionSnapshot::from_lockfile(lockfile.clone(), &api)
-              .await
-              .with_context(|| {
-                format!(
-                  "failed reading lockfile '{}'",
-                  lockfile.lock().filename.display()
-                )
-              })?,
-          )
-        }
-      }
-    }
-
-    Ok(Self::new_inner(
-      cache,
-      api,
-      local_node_modules_path,
-      initial_snapshot,
-      maybe_lockfile,
-    ))
-  }
-
-  fn new_inner(
-    cache: NpmCache,
-    api: NpmRegistryApi,
-    local_node_modules_path: Option<PathBuf>,
-    maybe_snapshot: Option<NpmResolutionSnapshot>,
+  pub fn new(
+    resolution: NpmResolution,
+    fs_resolver: Arc<dyn NpmPackageFsResolver>,
     maybe_lockfile: Option<Arc<Mutex<Lockfile>>>,
   ) -> Self {
-    let registry_url = api.base_url().to_owned();
-    let resolution =
-      NpmResolution::new(api.clone(), maybe_snapshot, maybe_lockfile.clone());
-    let fs_resolver: Arc<dyn NpmPackageFsResolver> =
-      match &local_node_modules_path {
-        Some(node_modules_folder) => Arc::new(LocalNpmPackageResolver::new(
-          cache.clone(),
-          registry_url,
-          node_modules_folder.clone(),
-          resolution.clone(),
-        )),
-        None => Arc::new(GlobalNpmPackageResolver::new(
-          cache.clone(),
-          registry_url,
-          resolution.clone(),
-        )),
-      };
     Self {
       fs_resolver,
-      local_node_modules_path,
-      api,
       resolution,
-      cache,
       maybe_lockfile,
     }
   }
 
-  pub fn api(&self) -> &NpmRegistryApi {
-    &self.api
+  pub fn resolve_pkg_id_from_pkg_req(
+    &self,
+    req: &NpmPackageReq,
+  ) -> Result<NpmPackageId, AnyError> {
+    self.resolution.resolve_pkg_id_from_pkg_req(req)
   }
 
-  pub fn resolution(&self) -> &NpmResolution {
-    &self.resolution
+  pub fn pkg_req_ref_to_nv_ref(
+    &self,
+    req_ref: NpmPackageReqReference,
+  ) -> Result<NpmPackageNvReference, AnyError> {
+    self.resolution.pkg_req_ref_to_nv_ref(req_ref)
   }
 
   /// Resolves an npm package folder path from a Deno module.
@@ -259,24 +200,13 @@ impl NpmPackageResolver {
     serde_json::to_string(&NpmProcessState {
       snapshot: self.snapshot(),
       local_node_modules_path: self
-        .local_node_modules_path
-        .as_ref()
+        .fs_resolver
+        .node_modules_path()
         .map(|p| p.to_string_lossy().to_string()),
     })
     .unwrap()
   }
 
-  /// Gets a new resolver with a new snapshotted state.
-  pub fn snapshotted(&self) -> Self {
-    Self::new_inner(
-      self.cache.clone(),
-      self.api.clone(),
-      self.local_node_modules_path.clone(),
-      Some(self.snapshot()),
-      None,
-    )
-  }
-
   pub fn snapshot(&self) -> NpmResolutionSnapshot {
     self.resolution.snapshot()
   }
@@ -344,6 +274,27 @@ impl RequireNpmResolver for NpmPackageResolver {
   }
 }
 
+pub fn create_npm_fs_resolver(
+  cache: NpmCache,
+  registry_url: Url,
+  resolution: NpmResolution,
+  maybe_node_modules_path: Option<PathBuf>,
+) -> Arc<dyn NpmPackageFsResolver> {
+  match maybe_node_modules_path {
+    Some(node_modules_folder) => Arc::new(LocalNpmPackageResolver::new(
+      cache,
+      registry_url,
+      node_modules_folder,
+      resolution,
+    )),
+    None => Arc::new(GlobalNpmPackageResolver::new(
+      cache,
+      registry_url,
+      resolution,
+    )),
+  }
+}
+
 fn path_to_specifier(path: &Path) -> Result<ModuleSpecifier, AnyError> {
   match ModuleSpecifier::from_file_path(path.to_path_buf().clean()) {
     Ok(specifier) => Ok(specifier),

cli/proc_state.rs

@@ -24,9 +24,11 @@ use crate::graph_util::ModuleGraphContainer;
 use crate::http_util::HttpClient;
 use crate::node;
 use crate::node::NodeResolution;
+use crate::npm::create_npm_fs_resolver;
 use crate::npm::NpmCache;
 use crate::npm::NpmPackageResolver;
 use crate::npm::NpmRegistryApi;
+use crate::npm::NpmResolution;
 use crate::npm::PackageJsonDepsInstaller;
 use crate::resolver::CliGraphResolver;
 use crate::tools::check;
@@ -91,8 +93,10 @@ pub struct Inner {
   pub resolver: Arc<CliGraphResolver>,
   maybe_file_watcher_reporter: Option<FileWatcherReporter>,
   pub node_analysis_cache: NodeAnalysisCache,
+  pub npm_api: NpmRegistryApi,
   pub npm_cache: NpmCache,
   pub npm_resolver: NpmPackageResolver,
+  pub npm_resolution: NpmResolution,
   pub package_json_deps_installer: PackageJsonDepsInstaller,
   pub cjs_resolutions: Mutex<HashSet<ModuleSpecifier>>,
   progress_bar: ProgressBar,
@@ -153,8 +157,10 @@ impl ProcState {
       resolver: self.resolver.clone(),
       maybe_file_watcher_reporter: self.maybe_file_watcher_reporter.clone(),
       node_analysis_cache: self.node_analysis_cache.clone(),
+      npm_api: self.npm_api.clone(),
       npm_cache: self.npm_cache.clone(),
       npm_resolver: self.npm_resolver.clone(),
+      npm_resolution: self.npm_resolution.clone(),
       package_json_deps_installer: self.package_json_deps_installer.clone(),
       cjs_resolutions: Default::default(),
       progress_bar: self.progress_bar.clone(),
@@ -210,30 +216,41 @@ impl ProcState {
 
     let lockfile = cli_options.maybe_lock_file();
 
-    let registry_url = NpmRegistryApi::default_url().to_owned();
+    let npm_registry_url = NpmRegistryApi::default_url().to_owned();
     let npm_cache = NpmCache::from_deno_dir(
       &dir,
       cli_options.cache_setting(),
       http_client.clone(),
       progress_bar.clone(),
     );
-    let api = NpmRegistryApi::new(
-      registry_url,
+    let npm_api = NpmRegistryApi::new(
+      npm_registry_url.clone(),
       npm_cache.clone(),
       http_client.clone(),
       progress_bar.clone(),
     );
-    let npm_resolver = NpmPackageResolver::new_with_maybe_lockfile(
-      npm_cache.clone(),
-      api,
-      cli_options.node_modules_dir_path(),
-      cli_options.get_npm_resolution_snapshot(),
+    let npm_snapshot = cli_options
+      .resolve_npm_resolution_snapshot(&npm_api)
+      .await?;
+    let npm_resolution = NpmResolution::new(
+      npm_api.clone(),
+      npm_snapshot,
       lockfile.as_ref().cloned(),
-    )
-    .await?;
+    );
+    let npm_fs_resolver = create_npm_fs_resolver(
+      npm_cache,
+      npm_registry_url,
+      npm_resolution.clone(),
+      cli_options.node_modules_dir_path(),
+    );
+    let npm_resolver = NpmPackageResolver::new(
+      npm_resolution.clone(),
+      npm_fs_resolver,
+      lockfile.as_ref().cloned(),
+    );
     let package_json_deps_installer = PackageJsonDepsInstaller::new(
-      npm_resolver.api().clone(),
-      npm_resolver.resolution().clone(),
+      npm_api.clone(),
+      npm_resolution.clone(),
       cli_options.maybe_package_json_deps(),
     );
     let maybe_import_map = cli_options
@@ -247,8 +264,8 @@ impl ProcState {
       cli_options.to_maybe_jsx_import_source_config(),
       maybe_import_map.clone(),
       cli_options.no_npm(),
-      npm_resolver.api().clone(),
-      npm_resolver.resolution().clone(),
+      npm_api.clone(),
+      npm_resolution.clone(),
       package_json_deps_installer.clone(),
     ));
 
@@ -299,8 +316,10 @@ impl ProcState {
       resolver,
       maybe_file_watcher_reporter,
       node_analysis_cache,
+      npm_api,
       npm_cache,
       npm_resolver,
+      npm_resolution,
       package_json_deps_installer,
       cjs_resolutions: Default::default(),
       progress_bar,
@@ -564,10 +583,8 @@ impl ProcState {
         if let Ok(reference) =
           NpmPackageReqReference::from_specifier(&specifier)
         {
-          let reference = self
-            .npm_resolver
-            .resolution()
-            .pkg_req_ref_to_nv_ref(reference)?;
+          let reference =
+            self.npm_resolution.pkg_req_ref_to_nv_ref(reference)?;
           return self
             .handle_node_resolve_result(node::node_resolve_npm_reference(
               &reference,
@@ -641,8 +658,8 @@ impl ProcState {
       self.options.to_maybe_jsx_import_source_config(),
       self.maybe_import_map.clone(),
       self.options.no_npm(),
-      self.npm_resolver.api().clone(),
-      self.npm_resolver.resolution().clone(),
+      self.npm_api.clone(),
+      self.npm_resolution.clone(),
       self.package_json_deps_installer.clone(),
     );
     let graph_resolver = cli_resolver.as_graph_resolver();

cli/standalone.rs

@@ -241,8 +241,8 @@ pub async fn run(
             parse_from_json(&base, &source).unwrap().import_map,
           )),
           false,
-          ps.npm_resolver.api().clone(),
-          ps.npm_resolver.resolution().clone(),
+          ps.npm_api.clone(),
+          ps.npm_resolution.clone(),
           ps.package_json_deps_installer.clone(),
         )
       },

cli/tsc/mod.rs

@@ -734,9 +734,7 @@ fn resolve_non_graph_specifier_types(
     // we don't need this special code here.
     // This could occur when resolving npm:@types/node when it is
     // injected and not part of the graph
-    let node_id = npm_resolver
-      .resolution()
-      .resolve_pkg_id_from_pkg_req(&npm_ref.req)?;
+    let node_id = npm_resolver.resolve_pkg_id_from_pkg_req(&npm_ref.req)?;
     let npm_id_ref = NpmPackageNvReference {
       nv: node_id.nv,
       sub_path: npm_ref.sub_path,

cli/worker.rs

@@ -448,8 +448,7 @@ async fn create_main_worker_internal(
       .add_package_reqs(vec![package_ref.req.clone()])
       .await?;
     let pkg_nv = ps
-      .npm_resolver
-      .resolution()
+      .npm_resolution
       .resolve_pkg_id_from_pkg_req(&package_ref.req)?
       .nv;
     let node_resolution = node::node_resolve_binary_export(