From b34e751a5b2193e8ce65203386e00147c08a7a64 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 15 Feb 2023 11:30:54 -0500 Subject: [PATCH] refactor: make resolver required (#17783) Makes the resolver required and prints a warning when vendoring and a dynamic import can't be resolved. Closes #16522 --- Cargo.lock | 6 +- cli/Cargo.toml | 2 +- cli/errors.rs | 3 +- cli/graph_util.rs | 88 +++++++++++++----- cli/lsp/documents.rs | 89 ++++++++----------- cli/lsp/language_server.rs | 8 +- cli/main.rs | 2 +- cli/proc_state.rs | 43 ++++----- cli/resolver.rs | 35 +++----- cli/standalone.rs | 13 +-- cli/tests/integration/run_tests.rs | 3 + cli/tests/integration/vendor_tests.rs | 8 ++ .../testdata/vendor/dynamic_non_existent.ts | 11 +++ .../vendor/dynamic_non_existent.ts.out | 7 ++ cli/tools/info.rs | 3 +- cli/tools/repl/session.rs | 6 +- cli/tools/vendor/build.rs | 5 +- cli/tools/vendor/import_map.rs | 7 +- cli/tools/vendor/test.rs | 4 +- 19 files changed, 197 insertions(+), 146 deletions(-) create mode 100644 cli/tests/testdata/vendor/dynamic_non_existent.ts create mode 100644 cli/tests/testdata/vendor/dynamic_non_existent.ts.out diff --git a/Cargo.lock b/Cargo.lock index 40d06b19de..9407a613fa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1092,19 +1092,21 @@ dependencies = [ [[package]] name = "deno_graph" -version = "0.43.0" +version = "0.43.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f71bb48b14950d62552aee1d05d0b90c203319b293b36358021ec77e05eaf9a4" +checksum = "9bfe0ba0ce8e54d198821714439a46b97c97de112d8a53a9a53d87c400da578c" dependencies = [ "anyhow", "data-url", "deno_ast", "futures", + "monch", "once_cell", "parking_lot 0.12.1", "regex", "serde", "serde_json", + "thiserror", "url", ] diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 5426777380..8fa43b8dfc 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -46,7 +46,7 @@ deno_ast = { workspace = true, features = ["bundler", "cjs", "codegen", "dep_gra deno_core.workspace = true deno_doc = "0.55.0" deno_emit = "0.15.0" -deno_graph = "0.43.0" +deno_graph = "0.43.1" deno_lint = { version = "0.38.0", features = ["docs"] } deno_lockfile.workspace = true deno_runtime.workspace = true diff --git a/cli/errors.rs b/cli/errors.rs index eb7282265b..4dc22efbf4 100644 --- a/cli/errors.rs +++ b/cli/errors.rs @@ -35,7 +35,8 @@ fn get_module_graph_error_class(err: &ModuleGraphError) -> &'static str { ModuleGraphError::ResolutionError(err) => get_resolution_error_class(err), ModuleGraphError::UnsupportedMediaType { .. } | ModuleGraphError::UnsupportedImportAssertionType { .. } => "TypeError", - ModuleGraphError::Missing(_, _) => "NotFound", + ModuleGraphError::Missing(_, _) + | ModuleGraphError::MissingDynamic(_, _) => "NotFound", } } diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 3fa849a713..0e6b308e1c 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -10,7 +10,7 @@ use crate::colors; use crate::errors::get_error_class_name; use crate::npm::resolve_graph_npm_info; use crate::proc_state::ProcState; -use crate::resolver::CliResolver; +use crate::resolver::CliGraphResolver; use crate::tools::check; use deno_core::anyhow::bail; @@ -25,6 +25,13 @@ use deno_runtime::permissions::PermissionsContainer; use import_map::ImportMapError; use std::sync::Arc; +#[derive(Clone, Copy)] +pub struct GraphValidOptions { + pub check_js: bool, + pub follow_type_only: bool, + pub is_vendoring: bool, +} + /// Check if `roots` and their deps are available. Returns `Ok(())` if /// so. Returns `Err(_)` if there is a known module graph or resolution /// error statically reachable from `roots` and not a dynamic import. @@ -36,8 +43,8 @@ pub fn graph_valid_with_cli_options( graph_valid( graph, roots, - deno_graph::WalkOptions { - follow_dynamic: false, + GraphValidOptions { + is_vendoring: false, follow_type_only: options.type_check_mode() != TypeCheckMode::None, check_js: options.check_js(), }, @@ -54,27 +61,61 @@ pub fn graph_valid_with_cli_options( pub fn graph_valid( graph: &ModuleGraph, roots: &[ModuleSpecifier], - walk_options: deno_graph::WalkOptions, + options: GraphValidOptions, ) -> Result<(), AnyError> { - graph.walk(roots, walk_options).validate().map_err(|error| { - let is_root = match &error { - ModuleGraphError::ResolutionError(_) => false, - _ => roots.contains(error.specifier()), - }; - let mut message = if let ModuleGraphError::ResolutionError(err) = &error { - enhanced_resolution_error_message(err) - } else { - format!("{error}") - }; + let mut errors = graph + .walk( + roots, + deno_graph::WalkOptions { + check_js: options.check_js, + follow_type_only: options.follow_type_only, + follow_dynamic: options.is_vendoring, + }, + ) + .errors() + .flat_map(|error| { + let is_root = match &error { + ModuleGraphError::ResolutionError(_) => false, + _ => roots.contains(error.specifier()), + }; + let mut message = if let ModuleGraphError::ResolutionError(err) = &error { + enhanced_resolution_error_message(err) + } else { + format!("{error}") + }; - if let Some(range) = error.maybe_range() { - if !is_root && !range.specifier.as_str().contains("/$deno$eval") { - message.push_str(&format!("\n at {range}")); + if let Some(range) = error.maybe_range() { + if !is_root && !range.specifier.as_str().contains("/$deno$eval") { + message.push_str(&format!("\n at {range}")); + } } - } - custom_error(get_error_class_name(&error.into()), message) - }) + if options.is_vendoring { + // warn about failing dynamic imports when vendoring, but don't fail completely + if matches!(error, ModuleGraphError::MissingDynamic(_, _)) { + log::warn!("Ignoring: {:#}", message); + return None; + } + + // ignore invalid downgrades and invalid local imports when vendoring + if let ModuleGraphError::ResolutionError(err) = &error { + if matches!( + err, + ResolutionError::InvalidDowngrade { .. } + | ResolutionError::InvalidLocalImport { .. } + ) { + return None; + } + } + } + + Some(custom_error(get_error_class_name(&error.into()), message)) + }); + if let Some(error) = errors.next() { + Err(error) + } else { + Ok(()) + } } /// Checks the lockfile against the graph and and exits on errors. @@ -109,12 +150,11 @@ pub async fn create_graph_and_maybe_check( PermissionsContainer::allow_all(), ); let maybe_imports = ps.options.to_maybe_imports()?; - let maybe_cli_resolver = CliResolver::maybe_new( + let cli_resolver = CliGraphResolver::new( ps.options.to_maybe_jsx_import_source_config(), ps.maybe_import_map.clone(), ); - let maybe_graph_resolver = - maybe_cli_resolver.as_ref().map(|r| r.as_graph_resolver()); + let graph_resolver = cli_resolver.as_graph_resolver(); let analyzer = ps.parsed_source_cache.as_analyzer(); let mut graph = ModuleGraph::default(); graph @@ -124,7 +164,7 @@ pub async fn create_graph_and_maybe_check( deno_graph::BuildOptions { is_dynamic: false, imports: maybe_imports, - resolver: maybe_graph_resolver, + resolver: Some(graph_resolver), module_analyzer: Some(&*analyzer), reporter: None, }, diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 329fc554b9..5faf09f5b8 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -19,7 +19,7 @@ use crate::node::NodeResolution; use crate::npm::NpmPackageReference; use crate::npm::NpmPackageReq; use crate::npm::NpmPackageResolver; -use crate::resolver::CliResolver; +use crate::resolver::CliGraphResolver; use crate::util::path::specifier_to_file_path; use crate::util::text_encoding; @@ -293,7 +293,7 @@ impl Document { fs_version: String, maybe_headers: Option>, text_info: SourceTextInfo, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> Self { // we only ever do `Document::new` on on disk resources that are supposed to // be diagnosable, unlike `Document::open`, so it is safe to unconditionally @@ -302,7 +302,7 @@ impl Document { &specifier, text_info.clone(), maybe_headers.as_ref(), - maybe_resolver, + resolver, ); let dependencies = Arc::new(DocumentDependencies::from_maybe_module(&maybe_module)); @@ -324,7 +324,7 @@ impl Document { fn maybe_with_new_resolver( &self, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> Option { let parsed_source_result = match &self.0.maybe_parsed_source { Some(parsed_source_result) => parsed_source_result.clone(), @@ -334,7 +334,7 @@ impl Document { &self.0.specifier, &parsed_source_result, self.0.maybe_headers.as_ref(), - maybe_resolver, + resolver, )); let dependencies = Arc::new(DocumentDependencies::from_maybe_module(&maybe_module)); @@ -360,7 +360,7 @@ impl Document { version: i32, language_id: LanguageId, content: Arc, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> Self { let maybe_headers = language_id.as_headers(); let text_info = SourceTextInfo::new(content); @@ -369,7 +369,7 @@ impl Document { &specifier, text_info.clone(), maybe_headers, - maybe_resolver, + resolver, ) } else { (None, None) @@ -396,7 +396,7 @@ impl Document { &self, version: i32, changes: Vec, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> Result { let mut content = self.0.text_info.text_str().to_string(); let mut line_index = self.0.line_index.clone(); @@ -431,7 +431,7 @@ impl Document { &self.0.specifier, text_info.clone(), maybe_headers, - maybe_resolver, + resolver, ) } else { (None, None) @@ -715,7 +715,7 @@ impl FileSystemDocuments { pub fn get( &mut self, cache: &HttpCache, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, specifier: &ModuleSpecifier, ) -> Option { let fs_version = get_document_path(cache, specifier) @@ -723,7 +723,7 @@ impl FileSystemDocuments { let file_system_doc = self.docs.get(specifier); if file_system_doc.map(|d| d.fs_version().to_string()) != fs_version { // attempt to update the file on the file system - self.refresh_document(cache, maybe_resolver, specifier) + self.refresh_document(cache, resolver, specifier) } else { file_system_doc.cloned() } @@ -734,7 +734,7 @@ impl FileSystemDocuments { fn refresh_document( &mut self, cache: &HttpCache, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, specifier: &ModuleSpecifier, ) -> Option { let path = get_document_path(cache, specifier)?; @@ -749,7 +749,7 @@ impl FileSystemDocuments { fs_version, None, SourceTextInfo::from_string(content), - maybe_resolver, + resolver, ) } else { let cache_filename = cache.get_cache_filename(specifier)?; @@ -763,7 +763,7 @@ impl FileSystemDocuments { fs_version, maybe_headers, SourceTextInfo::from_string(content), - maybe_resolver, + resolver, ) }; self.dirty = true; @@ -773,10 +773,10 @@ impl FileSystemDocuments { pub fn refresh_dependencies( &mut self, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) { for doc in self.docs.values_mut() { - if let Some(new_doc) = doc.maybe_with_new_resolver(maybe_resolver) { + if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { *doc = new_doc; } } @@ -817,7 +817,7 @@ pub struct Documents { imports: Arc>, /// A resolver that takes into account currently loaded import map and JSX /// settings. - maybe_resolver: Option, + resolver: CliGraphResolver, /// The npm package requirements. npm_reqs: Arc>, /// Gets if any document had a node: specifier such that a @types/node package @@ -837,7 +837,7 @@ impl Documents { file_system_docs: Default::default(), resolver_config_hash: 0, imports: Default::default(), - maybe_resolver: None, + resolver: CliGraphResolver::default(), npm_reqs: Default::default(), has_injected_types_node_package: false, specifier_resolver: Arc::new(SpecifierResolver::new(location)), @@ -855,13 +855,13 @@ impl Documents { language_id: LanguageId, content: Arc, ) -> Document { - let maybe_resolver = self.get_maybe_resolver(); + let resolver = self.get_resolver(); let document = Document::open( specifier.clone(), version, language_id, content, - maybe_resolver, + resolver, ); let mut file_system_docs = self.file_system_docs.lock(); file_system_docs.docs.remove(&specifier); @@ -896,7 +896,7 @@ impl Documents { Ok, )?; self.dirty = true; - let doc = doc.with_change(version, changes, self.get_maybe_resolver())?; + let doc = doc.with_change(version, changes, self.get_resolver())?; self.open_docs.insert(doc.specifier().clone(), doc.clone()); Ok(doc) } @@ -929,12 +929,7 @@ impl Documents { specifier: &str, referrer: &ModuleSpecifier, ) -> bool { - let maybe_resolver = self.get_maybe_resolver(); - let maybe_specifier = if let Some(resolver) = maybe_resolver { - resolver.resolve(specifier, referrer).ok() - } else { - deno_core::resolve_import(specifier, referrer.as_str()).ok() - }; + let maybe_specifier = self.get_resolver().resolve(specifier, referrer).ok(); if let Some(import_specifier) = maybe_specifier { self.exists(&import_specifier) } else { @@ -993,7 +988,7 @@ impl Documents { Some(document.clone()) } else { let mut file_system_docs = self.file_system_docs.lock(); - file_system_docs.get(&self.cache, self.get_maybe_resolver(), &specifier) + file_system_docs.get(&self.cache, self.get_resolver(), &specifier) } } @@ -1186,8 +1181,7 @@ impl Documents { maybe_import_map.as_deref(), maybe_jsx_config.as_ref(), ); - self.maybe_resolver = - CliResolver::maybe_new(maybe_jsx_config, maybe_import_map); + self.resolver = CliGraphResolver::new(maybe_jsx_config, maybe_import_map); self.imports = Arc::new( if let Some(Ok(imports)) = maybe_config_file.map(|cf| cf.to_maybe_imports()) @@ -1198,7 +1192,7 @@ impl Documents { let graph_import = GraphImport::new( &import.referrer, import.imports, - self.get_maybe_resolver(), + Some(self.get_resolver()), ); (import.referrer, graph_import) }) @@ -1218,17 +1212,13 @@ impl Documents { } fn refresh_dependencies(&mut self) { - let maybe_resolver = - self.maybe_resolver.as_ref().map(|r| r.as_graph_resolver()); + let resolver = self.resolver.as_graph_resolver(); for doc in self.open_docs.values_mut() { - if let Some(new_doc) = doc.maybe_with_new_resolver(maybe_resolver) { + if let Some(new_doc) = doc.maybe_with_new_resolver(resolver) { *doc = new_doc; } } - self - .file_system_docs - .lock() - .refresh_dependencies(maybe_resolver); + self.file_system_docs.lock().refresh_dependencies(resolver); } /// Iterate through the documents, building a map where the key is a unique @@ -1295,10 +1285,9 @@ impl Documents { doc_analyzer.analyze_doc(specifier, doc); } - let maybe_resolver = self.get_maybe_resolver(); + let resolver = self.get_resolver(); while let Some(specifier) = doc_analyzer.pending_specifiers.pop_front() { - if let Some(doc) = - file_system_docs.get(&self.cache, maybe_resolver, &specifier) + if let Some(doc) = file_system_docs.get(&self.cache, resolver, &specifier) { doc_analyzer.analyze_doc(&specifier, &doc); } @@ -1321,8 +1310,8 @@ impl Documents { file_system_docs.dirty = false; } - fn get_maybe_resolver(&self) -> Option<&dyn deno_graph::source::Resolver> { - self.maybe_resolver.as_ref().map(|r| r.as_graph_resolver()) + fn get_resolver(&self) -> &dyn deno_graph::source::Resolver { + self.resolver.as_graph_resolver() } fn resolve_dependency( @@ -1403,15 +1392,11 @@ fn parse_and_analyze_module( specifier: &ModuleSpecifier, text_info: SourceTextInfo, maybe_headers: Option<&HashMap>, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> (Option, Option) { let parsed_source_result = parse_source(specifier, text_info, maybe_headers); - let module_result = analyze_module( - specifier, - &parsed_source_result, - maybe_headers, - maybe_resolver, - ); + let module_result = + analyze_module(specifier, &parsed_source_result, maybe_headers, resolver); (Some(parsed_source_result), Some(module_result)) } @@ -1434,7 +1419,7 @@ fn analyze_module( specifier: &ModuleSpecifier, parsed_source_result: &ParsedSourceResult, maybe_headers: Option<&HashMap>, - maybe_resolver: Option<&dyn deno_graph::source::Resolver>, + resolver: &dyn deno_graph::source::Resolver, ) -> ModuleResult { match parsed_source_result { Ok(parsed_source) => Ok(deno_graph::parse_module_from_ast( @@ -1442,7 +1427,7 @@ fn analyze_module( deno_graph::ModuleKind::Esm, maybe_headers, parsed_source, - maybe_resolver, + Some(resolver), )), Err(err) => Err(deno_graph::ModuleGraphError::ParseErr( specifier.clone(), diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index f4045a19e8..70797eaf22 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -69,7 +69,7 @@ use crate::args::TsConfig; use crate::cache::DenoDir; use crate::cache::HttpCache; use crate::file_fetcher::FileFetcher; -use crate::graph_util::graph_valid; +use crate::graph_util; use crate::http_util::HttpClient; use crate::npm::NpmCache; use crate::npm::NpmPackageResolver; @@ -176,11 +176,11 @@ impl LanguageServer { let graph = ps .create_graph_with_loader(roots.clone(), &mut loader) .await?; - graph_valid( + graph_util::graph_valid( &graph, &roots, - deno_graph::WalkOptions { - follow_dynamic: false, + graph_util::GraphValidOptions { + is_vendoring: false, follow_type_only: true, check_js: false, }, diff --git a/cli/main.rs b/cli/main.rs index 71e2c202b7..d9f38876b1 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -30,7 +30,7 @@ use crate::args::flags_from_vec; use crate::args::DenoSubcommand; use crate::args::Flags; use crate::proc_state::ProcState; -use crate::resolver::CliResolver; +use crate::resolver::CliGraphResolver; use crate::util::display; use crate::util::v8::get_v8_flags_from_env; use crate::util::v8::init_v8_flags; diff --git a/cli/proc_state.rs b/cli/proc_state.rs index 0ab6b37544..077f328767 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -28,7 +28,7 @@ use crate::npm::NpmPackageReference; use crate::npm::NpmPackageReq; use crate::npm::NpmPackageResolver; use crate::npm::RealNpmRegistryApi; -use crate::resolver::CliResolver; +use crate::resolver::CliGraphResolver; use crate::tools::check; use crate::util::progress_bar::ProgressBar; use crate::util::progress_bar::ProgressBarStyle; @@ -58,6 +58,7 @@ use deno_runtime::inspector_server::InspectorServer; use deno_runtime::permissions::PermissionsContainer; use import_map::ImportMap; use log::warn; +use std::borrow::Cow; use std::collections::HashMap; use std::collections::HashSet; use std::ops::Deref; @@ -90,7 +91,7 @@ pub struct Inner { pub shared_array_buffer_store: SharedArrayBufferStore, pub compiled_wasm_module_store: CompiledWasmModuleStore, pub parsed_source_cache: ParsedSourceCache, - pub maybe_resolver: Option>, + pub resolver: Arc, maybe_file_watcher_reporter: Option, pub node_analysis_cache: NodeAnalysisCache, pub npm_cache: NpmCache, @@ -152,7 +153,7 @@ impl ProcState { shared_array_buffer_store: Default::default(), compiled_wasm_module_store: Default::default(), parsed_source_cache: self.parsed_source_cache.reset_for_file_watcher(), - maybe_resolver: self.maybe_resolver.clone(), + resolver: self.resolver.clone(), maybe_file_watcher_reporter: self.maybe_file_watcher_reporter.clone(), node_analysis_cache: self.node_analysis_cache.clone(), npm_cache: self.npm_cache.clone(), @@ -219,11 +220,10 @@ impl ProcState { let maybe_inspector_server = cli_options.resolve_inspector_server().map(Arc::new); - let maybe_cli_resolver = CliResolver::maybe_new( + let resolver = Arc::new(CliGraphResolver::new( cli_options.to_maybe_jsx_import_source_config(), maybe_import_map.clone(), - ); - let maybe_resolver = maybe_cli_resolver.map(Arc::new); + )); let maybe_file_watcher_reporter = maybe_sender.map(|sender| FileWatcherReporter { @@ -286,7 +286,7 @@ impl ProcState { shared_array_buffer_store, compiled_wasm_module_store, parsed_source_cache, - maybe_resolver, + resolver, maybe_file_watcher_reporter, node_analysis_cache, npm_cache, @@ -320,8 +320,7 @@ impl ProcState { dynamic_permissions, ); let maybe_imports = self.options.to_maybe_imports()?; - let maybe_resolver = - self.maybe_resolver.as_ref().map(|r| r.as_graph_resolver()); + let resolver = self.resolver.as_graph_resolver(); let maybe_file_watcher_reporter: Option<&dyn deno_graph::source::Reporter> = if let Some(reporter) = &self.maybe_file_watcher_reporter { Some(reporter) @@ -346,7 +345,7 @@ impl ProcState { deno_graph::BuildOptions { is_dynamic, imports: maybe_imports, - resolver: maybe_resolver, + resolver: Some(resolver), module_analyzer: Some(&*analyzer), reporter: maybe_file_watcher_reporter, }, @@ -589,12 +588,14 @@ impl ProcState { // FIXME(bartlomieju): this is another hack way to provide NPM specifier // support in REPL. This should be fixed. + let resolution = self.resolver.resolve(specifier, &referrer); + if is_repl { - let specifier = self - .maybe_resolver + let specifier = resolution .as_ref() - .and_then(|resolver| resolver.resolve(specifier, &referrer).ok()) - .or_else(|| ModuleSpecifier::parse(specifier).ok()); + .ok() + .map(Cow::Borrowed) + .or_else(|| ModuleSpecifier::parse(specifier).ok().map(Cow::Owned)); if let Some(specifier) = specifier { if let Ok(reference) = NpmPackageReference::from_specifier(&specifier) { return self @@ -609,12 +610,7 @@ impl ProcState { } } - if let Some(resolver) = &self.maybe_resolver { - resolver.resolve(specifier, &referrer) - } else { - deno_core::resolve_import(specifier, referrer.as_str()) - .map_err(|err| err.into()) - } + resolution } pub fn cache_module_emits(&self) -> Result<(), AnyError> { @@ -671,12 +667,11 @@ impl ProcState { ) -> Result { let maybe_imports = self.options.to_maybe_imports()?; - let maybe_cli_resolver = CliResolver::maybe_new( + let cli_resolver = CliGraphResolver::new( self.options.to_maybe_jsx_import_source_config(), self.maybe_import_map.clone(), ); - let maybe_graph_resolver = - maybe_cli_resolver.as_ref().map(|r| r.as_graph_resolver()); + let graph_resolver = cli_resolver.as_graph_resolver(); let analyzer = self.parsed_source_cache.as_analyzer(); let mut graph = ModuleGraph::default(); @@ -687,7 +682,7 @@ impl ProcState { deno_graph::BuildOptions { is_dynamic: false, imports: maybe_imports, - resolver: maybe_graph_resolver, + resolver: Some(graph_resolver), module_analyzer: Some(&*analyzer), reporter: None, }, diff --git a/cli/resolver.rs b/cli/resolver.rs index 817b5d3b08..11b2d874c1 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -1,7 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. use deno_core::error::AnyError; -use deno_core::resolve_import; use deno_core::ModuleSpecifier; use deno_graph::source::Resolver; use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE; @@ -13,41 +12,33 @@ use crate::args::JsxImportSourceConfig; /// A resolver that takes care of resolution, taking into account loaded /// import map, JSX settings. #[derive(Debug, Clone, Default)] -pub struct CliResolver { +pub struct CliGraphResolver { maybe_import_map: Option>, maybe_default_jsx_import_source: Option, maybe_jsx_import_source_module: Option, } -impl CliResolver { - pub fn maybe_new( +impl CliGraphResolver { + pub fn new( maybe_jsx_import_source_config: Option, maybe_import_map: Option>, - ) -> Option { - if maybe_jsx_import_source_config.is_some() || maybe_import_map.is_some() { - Some(Self { - maybe_import_map, - maybe_default_jsx_import_source: maybe_jsx_import_source_config - .as_ref() - .and_then(|c| c.default_specifier.clone()), - maybe_jsx_import_source_module: maybe_jsx_import_source_config - .map(|c| c.module), - }) - } else { - None + ) -> Self { + Self { + maybe_import_map, + maybe_default_jsx_import_source: maybe_jsx_import_source_config + .as_ref() + .and_then(|c| c.default_specifier.clone()), + maybe_jsx_import_source_module: maybe_jsx_import_source_config + .map(|c| c.module), } } - pub fn with_import_map(import_map: Arc) -> Self { - Self::maybe_new(None, Some(import_map)).unwrap() - } - pub fn as_graph_resolver(&self) -> &dyn Resolver { self } } -impl Resolver for CliResolver { +impl Resolver for CliGraphResolver { fn default_jsx_import_source(&self) -> Option { self.maybe_default_jsx_import_source.clone() } @@ -69,7 +60,7 @@ impl Resolver for CliResolver { .resolve(specifier, referrer) .map_err(|err| err.into()) } else { - resolve_import(specifier, referrer.as_str()).map_err(|err| err.into()) + deno_graph::resolve_import(specifier, referrer).map_err(|err| err.into()) } } } diff --git a/cli/standalone.rs b/cli/standalone.rs index e36584d720..c3a74dc3b5 100644 --- a/cli/standalone.rs +++ b/cli/standalone.rs @@ -8,7 +8,7 @@ use crate::ops; use crate::proc_state::ProcState; use crate::util::v8::construct_v8_flags; use crate::version; -use crate::CliResolver; +use crate::CliGraphResolver; use deno_core::anyhow::Context; use deno_core::error::type_error; use deno_core::error::AnyError; @@ -127,7 +127,7 @@ fn u64_from_bytes(arr: &[u8]) -> Result { struct EmbeddedModuleLoader { eszip: eszip::EszipV2, - maybe_import_map_resolver: Option, + maybe_import_map_resolver: Option, } impl ModuleLoader for EmbeddedModuleLoader { @@ -235,9 +235,12 @@ pub async fn run( eszip, maybe_import_map_resolver: metadata.maybe_import_map.map( |(base, source)| { - CliResolver::with_import_map(Arc::new( - parse_from_json(&base, &source).unwrap().import_map, - )) + CliGraphResolver::new( + None, + Some(Arc::new( + parse_from_json(&base, &source).unwrap().import_map, + )), + ) }, ), }); diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 3f2b8d81fc..6baa408e9a 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -3379,6 +3379,9 @@ async fn test_resolve_dns() { .unwrap(); let err = String::from_utf8_lossy(&output.stderr); let out = String::from_utf8_lossy(&output.stdout); + if !output.status.success() { + eprintln!("stderr: {err}"); + } assert!(output.status.success()); assert!(err.starts_with("Check file")); diff --git a/cli/tests/integration/vendor_tests.rs b/cli/tests/integration/vendor_tests.rs index cd2f7f12e2..1f159fe802 100644 --- a/cli/tests/integration/vendor_tests.rs +++ b/cli/tests/integration/vendor_tests.rs @@ -478,6 +478,14 @@ fn dynamic_non_analyzable_import() { assert!(output.status.success()); } +itest!(dynamic_non_existent { + args: "vendor http://localhost:4545/vendor/dynamic_non_existent.ts", + temp_cwd: true, + exit_code: 0, + http_server: true, + output: "vendor/dynamic_non_existent.ts.out", +}); + #[test] fn update_existing_config_test() { let _server = http_server(); diff --git a/cli/tests/testdata/vendor/dynamic_non_existent.ts b/cli/tests/testdata/vendor/dynamic_non_existent.ts new file mode 100644 index 0000000000..a48e2accb0 --- /dev/null +++ b/cli/tests/testdata/vendor/dynamic_non_existent.ts @@ -0,0 +1,11 @@ +// this should still vendor +// deno-lint-ignore no-constant-condition +if (false) { + await import("./non-existent.js"); +} + +export class Logger { + log(text: string) { + console.log(text); + } +} diff --git a/cli/tests/testdata/vendor/dynamic_non_existent.ts.out b/cli/tests/testdata/vendor/dynamic_non_existent.ts.out new file mode 100644 index 0000000000..a1b2ade81f --- /dev/null +++ b/cli/tests/testdata/vendor/dynamic_non_existent.ts.out @@ -0,0 +1,7 @@ +Download http://localhost:4545/vendor/dynamic_non_existent.ts +Download http://localhost:4545/vendor/non-existent.js +Ignoring: Dynamic import not found "http://localhost:4545/vendor/non-existent.js". + at http://localhost:4545/vendor/dynamic_non_existent.ts:4:16 +Vendored 1 module into vendor/ directory. + +To use vendored modules, specify the `--import-map vendor/import_map.json` flag when invoking Deno subcommands or add an `"importMap": ""` entry to a deno.json file. diff --git a/cli/tools/info.rs b/cli/tools/info.rs index 1e09d58cb4..317befceeb 100644 --- a/cli/tools/info.rs +++ b/cli/tools/info.rs @@ -618,7 +618,8 @@ impl<'a> GraphDisplayContext<'a> { ModuleGraphError::UnsupportedMediaType { .. } => { self.build_error_msg(specifier, "(unsupported)") } - ModuleGraphError::Missing(_, _) => { + ModuleGraphError::Missing(_, _) + | ModuleGraphError::MissingDynamic(_, _) => { self.build_error_msg(specifier, "(missing)") } } diff --git a/cli/tools/repl/session.rs b/cli/tools/repl/session.rs index 18b40a1b9e..843e985e67 100644 --- a/cli/tools/repl/session.rs +++ b/cli/tools/repl/session.rs @@ -444,9 +444,9 @@ impl ReplSession { .flat_map(|i| { self .proc_state - .maybe_resolver - .as_ref() - .and_then(|resolver| resolver.resolve(i, &self.referrer).ok()) + .resolver + .resolve(i, &self.referrer) + .ok() .or_else(|| ModuleSpecifier::parse(i).ok()) }) .collect::>(); diff --git a/cli/tools/vendor/build.rs b/cli/tools/vendor/build.rs index f3cc01444e..89130f391b 100644 --- a/cli/tools/vendor/build.rs +++ b/cli/tools/vendor/build.rs @@ -82,10 +82,9 @@ pub fn build( graph_util::graph_valid( &graph, &graph.roots, - deno_graph::WalkOptions { - // surface all errors + graph_util::GraphValidOptions { + is_vendoring: true, check_js: true, - follow_dynamic: true, follow_type_only: true, }, )?; diff --git a/cli/tools/vendor/import_map.rs b/cli/tools/vendor/import_map.rs index 753ac52e5a..3d2c1efd93 100644 --- a/cli/tools/vendor/import_map.rs +++ b/cli/tools/vendor/import_map.rs @@ -290,7 +290,12 @@ fn handle_dep_specifier( referrer: &ModuleSpecifier, mappings: &Mappings, ) { - let specifier = graph.resolve(unresolved_specifier); + let specifier = match graph.get(unresolved_specifier) { + Some(module) => module.specifier.clone(), + // Ignore when None. The graph was previous validated so this is a + // dynamic import that was missing and is ignored for vendoring + None => return, + }; // check if it's referencing a remote module if is_remote_specifier(&specifier) { handle_remote_dep_specifier( diff --git a/cli/tools/vendor/test.rs b/cli/tools/vendor/test.rs index e5713a54c7..31df151f20 100644 --- a/cli/tools/vendor/test.rs +++ b/cli/tools/vendor/test.rs @@ -20,7 +20,7 @@ use deno_graph::ModuleGraph; use import_map::ImportMap; use crate::cache::ParsedSourceCache; -use crate::resolver::CliResolver; +use crate::resolver::CliGraphResolver; use super::build::VendorEnvironment; @@ -261,7 +261,7 @@ async fn build_test_graph( analyzer: &dyn deno_graph::ModuleAnalyzer, ) -> ModuleGraph { let resolver = - original_import_map.map(|m| CliResolver::with_import_map(Arc::new(m))); + original_import_map.map(|m| CliGraphResolver::new(None, Some(Arc::new(m)))); let mut graph = ModuleGraph::default(); graph .build(