From 828d9b84858d53b74d7801604c8c3bee418b631e Mon Sep 17 00:00:00 2001 From: David Sherret Date: Fri, 16 Feb 2024 17:10:06 -0500 Subject: [PATCH] refactor: remove module graph setup from ModuleLoader (#22442) `ModuleLoader` was doing too much duplicate work. --- cli/factory.rs | 4 -- cli/graph_util.rs | 142 ++++++++++++++++++-------------------- cli/module_loader.rs | 49 ++----------- cli/tools/registry/mod.rs | 1 + 4 files changed, 72 insertions(+), 124 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index cccbecbf18..2797afb294 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -657,12 +657,8 @@ impl CliFactory { self.fs().clone(), self.graph_container().clone(), self.maybe_lockfile().clone(), - self.maybe_file_watcher_reporter().clone(), self.module_graph_builder().await?.clone(), - self.module_info_cache()?.clone(), - self.parsed_source_cache().clone(), self.text_only_progress_bar().clone(), - self.resolver().await?.clone(), self.type_checker().await?.clone(), ))) }) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 09f0db9e64..b9027afa3f 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -206,6 +206,7 @@ pub fn graph_lock_or_exit(graph: &ModuleGraph, lockfile: &mut Lockfile) { pub struct CreateGraphOptions<'a> { pub graph_kind: GraphKind, pub roots: Vec, + pub is_dynamic: bool, /// Whether to do fast check on workspace members. This is mostly only /// useful when publishing. pub workspace_fast_check: bool, @@ -279,6 +280,7 @@ impl ModuleGraphBuilder { ) -> Result { self .create_graph_with_options(CreateGraphOptions { + is_dynamic: false, graph_kind, roots, loader: Some(loader), @@ -291,54 +293,10 @@ impl ModuleGraphBuilder { &self, options: CreateGraphOptions<'_>, ) -> Result { - enum MutLoaderRef<'a> { - Borrowed(&'a mut dyn Loader), - Owned(cache::FetchCacher), - } - - impl<'a> MutLoaderRef<'a> { - pub fn as_mut_loader(&mut self) -> &mut dyn Loader { - match self { - Self::Borrowed(loader) => *loader, - Self::Owned(loader) => loader, - } - } - } - - let parser = self.parsed_source_cache.as_capturing_parser(); - let analyzer = self.module_info_cache.as_module_analyzer(&parser); - let maybe_imports = self.options.to_maybe_imports()?; - let cli_resolver = self.resolver.clone(); - let graph_resolver = cli_resolver.as_graph_resolver(); - let graph_npm_resolver = cli_resolver.as_graph_npm_resolver(); - let maybe_file_watcher_reporter = self - .maybe_file_watcher_reporter - .as_ref() - .map(|r| r.as_reporter()); - let mut loader = match options.loader { - Some(loader) => MutLoaderRef::Borrowed(loader), - None => MutLoaderRef::Owned(self.create_graph_loader()), - }; - let mut graph = ModuleGraph::new(options.graph_kind); + self - .build_graph_with_npm_resolution( - &mut graph, - options.roots, - loader.as_mut_loader(), - deno_graph::BuildOptions { - is_dynamic: false, - imports: maybe_imports, - resolver: Some(graph_resolver), - file_system: Some(&DenoGraphFsAdapter(self.fs.as_ref())), - npm_resolver: Some(graph_npm_resolver), - module_analyzer: Some(&analyzer), - module_parser: Some(&parser), - reporter: maybe_file_watcher_reporter, - workspace_fast_check: options.workspace_fast_check, - workspace_members: self.get_deno_graph_workspace_members()?, - }, - ) + .build_graph_with_npm_resolution(&mut graph, options) .await?; if let Some(npm_resolver) = self.npm_resolver.as_managed() { @@ -354,38 +312,16 @@ impl ModuleGraphBuilder { &self, roots: Vec, ) -> Result, AnyError> { - let mut cache = self.create_graph_loader(); - let maybe_imports = self.options.to_maybe_imports()?; - let cli_resolver = self.resolver.clone(); - let graph_resolver = cli_resolver.as_graph_resolver(); - let graph_npm_resolver = cli_resolver.as_graph_npm_resolver(); - let parser = self.parsed_source_cache.as_capturing_parser(); - let analyzer = self.module_info_cache.as_module_analyzer(&parser); let graph_kind = self.options.type_check_mode().as_graph_kind(); - let mut graph = ModuleGraph::new(graph_kind); - let maybe_file_watcher_reporter = self - .maybe_file_watcher_reporter - .as_ref() - .map(|r| r.as_reporter()); - self - .build_graph_with_npm_resolution( - &mut graph, + let graph = self + .create_graph_with_options(CreateGraphOptions { + is_dynamic: false, + graph_kind, roots, - &mut cache, - deno_graph::BuildOptions { - is_dynamic: false, - imports: maybe_imports, - file_system: Some(&DenoGraphFsAdapter(self.fs.as_ref())), - resolver: Some(graph_resolver), - npm_resolver: Some(graph_npm_resolver), - module_analyzer: Some(&analyzer), - module_parser: Some(&parser), - reporter: maybe_file_watcher_reporter, - workspace_fast_check: false, - workspace_members: self.get_deno_graph_workspace_members()?, - }, - ) + loader: None, + workspace_fast_check: false, + }) .await?; let graph = Arc::new(graph); @@ -441,6 +377,60 @@ impl ModuleGraphBuilder { } pub async fn build_graph_with_npm_resolution<'a>( + &self, + graph: &mut ModuleGraph, + options: CreateGraphOptions<'a>, + ) -> Result<(), AnyError> { + enum MutLoaderRef<'a> { + Borrowed(&'a mut dyn Loader), + Owned(cache::FetchCacher), + } + + impl<'a> MutLoaderRef<'a> { + pub fn as_mut_loader(&mut self) -> &mut dyn Loader { + match self { + Self::Borrowed(loader) => *loader, + Self::Owned(loader) => loader, + } + } + } + + let maybe_imports = self.options.to_maybe_imports()?; + let parser = self.parsed_source_cache.as_capturing_parser(); + let analyzer = self.module_info_cache.as_module_analyzer(&parser); + let mut loader = match options.loader { + Some(loader) => MutLoaderRef::Borrowed(loader), + None => MutLoaderRef::Owned(self.create_graph_loader()), + }; + let cli_resolver = self.resolver.clone(); + let graph_resolver = cli_resolver.as_graph_resolver(); + let graph_npm_resolver = cli_resolver.as_graph_npm_resolver(); + let maybe_file_watcher_reporter = self + .maybe_file_watcher_reporter + .as_ref() + .map(|r| r.as_reporter()); + self + .build_graph_with_npm_resolution_and_build_options( + graph, + options.roots, + loader.as_mut_loader(), + deno_graph::BuildOptions { + is_dynamic: options.is_dynamic, + imports: maybe_imports, + resolver: Some(graph_resolver), + file_system: Some(&DenoGraphFsAdapter(self.fs.as_ref())), + npm_resolver: Some(graph_npm_resolver), + module_analyzer: Some(&analyzer), + module_parser: Some(&parser), + reporter: maybe_file_watcher_reporter, + workspace_fast_check: options.workspace_fast_check, + workspace_members: self.get_deno_graph_workspace_members()?, + }, + ) + .await + } + + async fn build_graph_with_npm_resolution_and_build_options<'a>( &self, graph: &mut ModuleGraph, roots: Vec, @@ -841,7 +831,7 @@ impl deno_graph::source::Reporter for FileWatcherReporter { } } -pub fn workspace_config_to_workspace_members( +fn workspace_config_to_workspace_members( workspace_config: &deno_config::WorkspaceConfig, ) -> Result, AnyError> { workspace_config diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 84ced345f7..4e0fe7142c 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -3,14 +3,11 @@ use crate::args::CliOptions; use crate::args::DenoSubcommand; use crate::args::TsTypeLib; -use crate::cache::ModuleInfoCache; use crate::cache::ParsedSourceCache; use crate::emit::Emitter; use crate::graph_util::graph_lock_or_exit; use crate::graph_util::graph_valid_with_cli_options; -use crate::graph_util::workspace_config_to_workspace_members; -use crate::graph_util::DenoGraphFsAdapter; -use crate::graph_util::FileWatcherReporter; +use crate::graph_util::CreateGraphOptions; use crate::graph_util::ModuleGraphBuilder; use crate::graph_util::ModuleGraphContainer; use crate::node; @@ -68,12 +65,8 @@ pub struct ModuleLoadPreparer { fs: Arc, graph_container: Arc, lockfile: Option>>, - maybe_file_watcher_reporter: Option, module_graph_builder: Arc, - module_info_cache: Arc, - parsed_source_cache: Arc, progress_bar: ProgressBar, - resolver: Arc, type_checker: Arc, } @@ -84,12 +77,8 @@ impl ModuleLoadPreparer { fs: Arc, graph_container: Arc, lockfile: Option>>, - maybe_file_watcher_reporter: Option, module_graph_builder: Arc, - module_info_cache: Arc, - parsed_source_cache: Arc, progress_bar: ProgressBar, - resolver: Arc, type_checker: Arc, ) -> Self { Self { @@ -97,12 +86,8 @@ impl ModuleLoadPreparer { fs, graph_container, lockfile, - maybe_file_watcher_reporter, module_graph_builder, - module_info_cache, - parsed_source_cache, progress_bar, - resolver, type_checker, } } @@ -123,23 +108,6 @@ impl ModuleLoadPreparer { let _pb_clear_guard = self.progress_bar.clear_guard(); let mut cache = self.module_graph_builder.create_fetch_cacher(permissions); - let maybe_imports = self.options.to_maybe_imports()?; - let maybe_workspace_config = self.options.maybe_workspace_config(); - let workspace_members = if let Some(wc) = maybe_workspace_config { - workspace_config_to_workspace_members(wc)? - } else { - vec![] - }; - let graph_resolver = self.resolver.as_graph_resolver(); - let graph_npm_resolver = self.resolver.as_graph_npm_resolver(); - let maybe_file_watcher_reporter = self - .maybe_file_watcher_reporter - .as_ref() - .map(|r| r.as_reporter()); - - let parser = self.parsed_source_cache.as_capturing_parser(); - let analyzer = self.module_info_cache.as_module_analyzer(&parser); - log::debug!("Creating module graph."); let mut graph_update_permit = self.graph_container.acquire_update_permit().await; @@ -154,19 +122,12 @@ impl ModuleLoadPreparer { .module_graph_builder .build_graph_with_npm_resolution( graph, - roots.clone(), - &mut cache, - deno_graph::BuildOptions { + CreateGraphOptions { is_dynamic, - imports: maybe_imports, - file_system: Some(&DenoGraphFsAdapter(self.fs.as_ref())), - resolver: Some(graph_resolver), - npm_resolver: Some(graph_npm_resolver), - module_parser: Some(&parser), - module_analyzer: Some(&analyzer), - reporter: maybe_file_watcher_reporter, + graph_kind: graph.graph_kind(), + roots: roots.clone(), + loader: Some(&mut cache), workspace_fast_check: false, - workspace_members, }, ) .await?; diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index dfd20f5715..9205d9b267 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -768,6 +768,7 @@ async fn build_and_check_graph_for_publish( let graph = Arc::new( module_graph_builder .create_graph_with_options(crate::graph_util::CreateGraphOptions { + is_dynamic: false, // All because we're going to use this same graph to determine the publish order later graph_kind: deno_graph::GraphKind::All, roots: packages