From bcb6ee9d0864f490f6da47cbe2593310b21333ff Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 12 Mar 2023 23:32:59 -0400 Subject: [PATCH] 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 | 25 ++++++- cli/graph_util.rs | 4 +- cli/lsp/diagnostics.rs | 2 - cli/lsp/documents.rs | 1 - cli/lsp/language_server.rs | 70 +++++++++++++++---- cli/npm/mod.rs | 1 + cli/npm/resolution/mod.rs | 3 + cli/npm/resolvers/common.rs | 3 + cli/npm/resolvers/global.rs | 4 ++ cli/npm/resolvers/local.rs | 4 ++ cli/npm/resolvers/mod.rs | 129 +++++++++++------------------------- cli/proc_state.rs | 57 ++++++++++------ cli/standalone.rs | 4 +- cli/tsc/mod.rs | 4 +- cli/worker.rs | 3 +- 15 files changed, 178 insertions(+), 136 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index 71cc4e2182..5be5fc7ab3 100644 --- a/cli/args/mod.rs +++ b/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 { + pub async fn resolve_npm_resolution_snapshot( + &self, + api: &NpmRegistryApi, + ) -> Result, 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. diff --git a/cli/graph_util.rs b/cli/graph_util.rs index ecae9ea4e2..0a94368339 100644 --- a/cli/graph_util.rs +++ b/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(); diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 380323dbaa..8c1c91da05 100644 --- a/cli/lsp/diagnostics.rs +++ b/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() { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index d49cd0b1fa..ff384bbf1a 100644 --- a/cli/lsp/documents.rs +++ b/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| { diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index 357d77adec..faad942950 100644 --- a/cli/lsp/language_server.rs +++ b/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, + /// 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(); diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index ea18f8866e..b1ce6fda4a 100644 --- a/cli/npm/mod.rs +++ b/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; diff --git a/cli/npm/resolution/mod.rs b/cli/npm/resolution/mod.rs index e1e3307c30..82dc1c62c9 100644 --- a/cli/npm/resolution/mod.rs +++ b/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); diff --git a/cli/npm/resolvers/common.rs b/cli/npm/resolvers/common.rs index 3f1c2a7047..a8e822bb98 100644 --- a/cli/npm/resolvers/common.rs +++ b/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; + fn resolve_package_folder_from_deno_module( &self, id: &NpmPackageId, diff --git a/cli/npm/resolvers/global.rs b/cli/npm/resolvers/global.rs index 87ad92675a..5d5334299f 100644 --- a/cli/npm/resolvers/global.rs +++ b/cli/npm/resolvers/global.rs @@ -72,6 +72,10 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { self.cache.root_dir_url() } + fn node_modules_path(&self) -> Option { + None + } + fn resolve_package_folder_from_deno_module( &self, id: &NpmPackageId, diff --git a/cli/npm/resolvers/local.rs b/cli/npm/resolvers/local.rs index 52a783823f..89f5decd85 100644 --- a/cli/npm/resolvers/local.rs +++ b/cli/npm/resolvers/local.rs @@ -128,6 +128,10 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver { &self.root_node_modules_url } + fn node_modules_path(&self) -> Option { + Some(self.root_node_modules_path.clone()) + } + fn resolve_package_folder_from_deno_module( &self, node_id: &NpmPackageId, diff --git a/cli/npm/resolvers/mod.rs b/cli/npm/resolvers/mod.rs index 0027698c0a..49a3c46f76 100644 --- a/cli/npm/resolvers/mod.rs +++ b/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, } +/// Brings together the npm resolution with the file system. #[derive(Clone)] pub struct NpmPackageResolver { fs_resolver: Arc, - local_node_modules_path: Option, - api: NpmRegistryApi, resolution: NpmResolution, - cache: NpmCache, maybe_lockfile: Option>>, } @@ -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", &"") - .field("local_node_modules_path", &self.local_node_modules_path) - .field("api", &"") .field("resolution", &"") - .field("cache", &"") .field("maybe_lockfile", &"") .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, - initial_snapshot: Option, - maybe_lockfile: Option>>, - ) -> Result { - 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, - maybe_snapshot: Option, + pub fn new( + resolution: NpmResolution, + fs_resolver: Arc, maybe_lockfile: Option>>, ) -> Self { - let registry_url = api.base_url().to_owned(); - let resolution = - NpmResolution::new(api.clone(), maybe_snapshot, maybe_lockfile.clone()); - let fs_resolver: Arc = - 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 { + 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 { + 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, +) -> Arc { + 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 { match ModuleSpecifier::from_file_path(path.to_path_buf().clean()) { Ok(specifier) => Ok(specifier), diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 6c3c407d8c..3d7130e5a0 100644 --- a/cli/proc_state.rs +++ b/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, maybe_file_watcher_reporter: Option, 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>, 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(); diff --git a/cli/standalone.rs b/cli/standalone.rs index c678dd37a8..6d2c7551a3 100644 --- a/cli/standalone.rs +++ b/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(), ) }, diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index fb9eeba0d8..2f9b4224c9 100644 --- a/cli/tsc/mod.rs +++ b/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, diff --git a/cli/worker.rs b/cli/worker.rs index 050891256e..33352a2d79 100644 --- a/cli/worker.rs +++ b/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(