From 63a821b78b5a293c0eb5b3ecc18d67bde8331eda Mon Sep 17 00:00:00 2001 From: Kitson Kelly Date: Tue, 15 Dec 2020 16:52:55 +1100 Subject: [PATCH] fix(cli): make dynamic import errors catchable (#8750) Fixes #6259 --- cli/disk_cache.rs | 24 +- cli/module_graph.rs | 420 ++++++++++++------ cli/module_loader.rs | 18 +- cli/program_state.rs | 136 +++--- cli/specifier_handler.rs | 86 ++-- cli/tests/dynamic_import/b.js | 2 + cli/tests/dynamic_import/c.js | 2 + cli/tests/error_004_missing_module.ts | 2 + .../error_005_missing_dynamic_import.ts.out | 3 +- .../error_015_dynamic_import_permissions.out | 3 +- cli/tests/fix_dynamic_import_errors.js | 7 + cli/tests/fix_dynamic_import_errors.js.out | 2 + cli/tests/integration_tests.rs | 5 + cli/tests/redirect_cache.out | 1 - .../unsupported_dynamic_import_scheme.out | 2 +- core/modules.rs | 2 +- 16 files changed, 446 insertions(+), 269 deletions(-) create mode 100644 cli/tests/dynamic_import/b.js create mode 100644 cli/tests/dynamic_import/c.js create mode 100644 cli/tests/fix_dynamic_import_errors.js create mode 100644 cli/tests/fix_dynamic_import_errors.js.out diff --git a/cli/disk_cache.rs b/cli/disk_cache.rs index 96a4ff41aa..233990903b 100644 --- a/cli/disk_cache.rs +++ b/cli/disk_cache.rs @@ -46,7 +46,7 @@ impl DiskCache { }) } - pub fn get_cache_filename(&self, url: &Url) -> PathBuf { + fn get_cache_filename(&self, url: &Url) -> Option { let mut out = PathBuf::new(); let scheme = url.scheme(); @@ -105,31 +105,25 @@ impl DiskCache { out = out.join(remaining_components); } - scheme => { - unimplemented!( - "Don't know how to create cache name for scheme: {}\n Url: {}", - scheme, - url - ); - } + _ => return None, }; - out + Some(out) } pub fn get_cache_filename_with_extension( &self, url: &Url, extension: &str, - ) -> PathBuf { - let base = self.get_cache_filename(url); + ) -> Option { + let base = self.get_cache_filename(url)?; match base.extension() { - None => base.with_extension(extension), + None => Some(base.with_extension(extension)), Some(ext) => { let original_extension = OsStr::to_str(ext).unwrap(); let final_extension = format!("{}.{}", original_extension, extension); - base.with_extension(final_extension) + Some(base.with_extension(final_extension)) } } } @@ -234,7 +228,7 @@ mod tests { for test_case in &test_cases { let cache_filename = cache.get_cache_filename(&Url::parse(test_case.0).unwrap()); - assert_eq!(cache_filename, PathBuf::from(test_case.1)); + assert_eq!(cache_filename, Some(PathBuf::from(test_case.1))); } } @@ -280,7 +274,7 @@ mod tests { &Url::parse(test_case.0).unwrap(), test_case.1 ), - PathBuf::from(test_case.2) + Some(PathBuf::from(test_case.2)) ) } } diff --git a/cli/module_graph.rs b/cli/module_graph.rs index 4144ee5ee9..f5e08882ed 100644 --- a/cli/module_graph.rs +++ b/cli/module_graph.rs @@ -28,6 +28,8 @@ use crate::tsc_config::TsConfig; use crate::version; use crate::AnyError; +use deno_core::error::anyhow; +use deno_core::error::custom_error; use deno_core::error::Context; use deno_core::futures::stream::FuturesUnordered; use deno_core::futures::stream::StreamExt; @@ -38,6 +40,7 @@ use deno_core::serde::Serializer; use deno_core::serde_json::json; use deno_core::serde_json::Value; use deno_core::ModuleResolutionError; +use deno_core::ModuleSource; use deno_core::ModuleSpecifier; use regex::Regex; use std::cell::RefCell; @@ -482,6 +485,10 @@ impl Module { Ok(specifier) } + pub fn set_emit(&mut self, code: String, maybe_map: Option) { + self.maybe_emit = Some(Emit::Cli((code, maybe_map))); + } + /// Calculate the hashed version of the module and update the `maybe_version`. pub fn set_version(&mut self, config: &[u8]) { self.maybe_version = @@ -523,6 +530,9 @@ pub struct ResultInfo { /// A structure which provides diagnostic information (usually from `tsc`) /// about the code in the module graph. pub diagnostics: Diagnostics, + /// A map of specifiers to the result of their resolution in the module graph. + pub loadable_modules: + HashMap>, /// Optionally ignored compiler options that represent any options that were /// ignored if there was a user provided configuration. pub maybe_ignored_options: Option, @@ -637,6 +647,18 @@ pub struct TranspileOptions { pub reload: bool, } +#[derive(Debug, Clone)] +enum ModuleSlot { + /// The module fetch resulted in a non-recoverable error. + Err(Rc), + /// The the fetch resulted in a module. + Module(Box), + /// Used to denote a module that isn't part of the graph. + None, + /// The fetch of the module is pending. + Pending, +} + /// A dependency graph of modules, were the modules that have been inserted via /// the builder will be loaded into the graph. Also provides an interface to /// be able to manipulate and handle the graph. @@ -649,7 +671,7 @@ pub struct Graph { /// invoked. maybe_tsbuildinfo: Option, /// The modules that are part of the graph. - modules: HashMap, + modules: HashMap, /// A map of redirects, where a module specifier is redirected to another /// module specifier by the handler. All modules references should be /// resolved internally via this, before attempting to access the module via @@ -667,6 +689,44 @@ pub struct Graph { maybe_lockfile: Option>>, } +/// Convert a specifier and a module slot in a result to the module source which +/// is needed by Deno core for loading the module. +fn to_module_result( + (specifier, module_slot): (&ModuleSpecifier, &ModuleSlot), +) -> (ModuleSpecifier, Result) { + match module_slot { + ModuleSlot::Err(err) => (specifier.clone(), Err(anyhow!(err.to_string()))), + ModuleSlot::Module(module) => ( + specifier.clone(), + if let Some(emit) = &module.maybe_emit { + match emit { + Emit::Cli((code, _)) => Ok(ModuleSource { + code: code.clone(), + module_url_found: module.specifier.to_string(), + module_url_specified: specifier.to_string(), + }), + } + } else { + match module.media_type { + MediaType::JavaScript | MediaType::Unknown => Ok(ModuleSource { + code: module.source.clone(), + module_url_found: module.specifier.to_string(), + module_url_specified: specifier.to_string(), + }), + _ => Err(custom_error( + "NotFound", + format!("Compiled module not found \"{}\"", specifier), + )), + } + }, + ), + _ => ( + specifier.clone(), + Err(anyhow!("Module \"{}\" unavailable.", specifier)), + ), + } +} + impl Graph { /// Create a new instance of a graph, ready to have modules loaded it. /// @@ -767,6 +827,7 @@ impl Graph { debug!("graph does not need to be checked or emitted."); return Ok(ResultInfo { maybe_ignored_options, + loadable_modules: self.get_loadable_modules(), ..Default::default() }); } @@ -834,9 +895,10 @@ impl Graph { } let config = config.as_bytes(); for (specifier, code) in codes.iter() { - if let Some(module) = graph.get_module_mut(specifier) { - module.maybe_emit = - Some(Emit::Cli((code.clone(), maps.get(specifier).cloned()))); + if let ModuleSlot::Module(module) = + graph.get_module_mut(specifier).unwrap() + { + module.set_emit(code.clone(), maps.get(specifier).cloned()); module.set_version(&config); module.is_dirty = true; } else { @@ -849,16 +911,12 @@ impl Graph { Ok(ResultInfo { diagnostics: response.diagnostics, + loadable_modules: graph.get_loadable_modules(), maybe_ignored_options, stats: response.stats, }) } - fn contains_module(&self, specifier: &ModuleSpecifier) -> bool { - let s = self.resolve_specifier(specifier); - self.modules.contains_key(s) - } - /// Emit the module graph in a specific format. This is specifically designed /// to be an "all-in-one" API for access by the runtime, allowing both /// emitting single modules as well as bundles, using Deno module resolution @@ -921,13 +979,13 @@ impl Graph { )?; let mut emitted_files = HashMap::new(); + let graph = graph.borrow(); match options.bundle_type { BundleType::Esm => { assert!( response.emitted_files.is_empty(), "No files should have been emitted from tsc." ); - let graph = graph.borrow(); assert_eq!( graph.roots.len(), 1, @@ -966,6 +1024,7 @@ impl Graph { emitted_files, ResultInfo { diagnostics: response.diagnostics, + loadable_modules: graph.get_loadable_modules(), maybe_ignored_options, stats: response.stats, }, @@ -1023,15 +1082,17 @@ impl Graph { /// any build info if present. fn flush(&mut self) -> Result<(), AnyError> { let mut handler = self.handler.borrow_mut(); - for (_, module) in self.modules.iter_mut() { - if module.is_dirty { - if let Some(emit) = &module.maybe_emit { - handler.set_cache(&module.specifier, emit)?; + for (_, module_slot) in self.modules.iter_mut() { + if let ModuleSlot::Module(module) = module_slot { + if module.is_dirty { + if let Some(emit) = &module.maybe_emit { + handler.set_cache(&module.specifier, emit)?; + } + if let Some(version) = &module.maybe_version { + handler.set_version(&module.specifier, version.clone())?; + } + module.is_dirty = false; } - if let Some(version) = &module.maybe_version { - handler.set_version(&module.specifier, version.clone())?; - } - module.is_dirty = false; } } for root_specifier in self.roots.iter() { @@ -1050,7 +1111,12 @@ impl Graph { totals: &mut HashMap, ) -> ModuleInfo { let not_seen = seen.insert(specifier.clone()); - let module = self.get_module(specifier).unwrap(); + let module = if let ModuleSlot::Module(module) = self.get_module(specifier) + { + module + } else { + unreachable!(); + }; let mut deps = Vec::new(); let mut total_size = None; @@ -1097,50 +1163,79 @@ impl Graph { let map = self .modules .iter() - .map(|(specifier, module)| { - let mut deps = BTreeSet::new(); - for (_, dep) in module.dependencies.iter() { - if let Some(code_dep) = &dep.maybe_code { - deps.insert(code_dep.clone()); + .filter_map(|(specifier, module_slot)| { + if let ModuleSlot::Module(module) = module_slot { + let mut deps = BTreeSet::new(); + for (_, dep) in module.dependencies.iter() { + if let Some(code_dep) = &dep.maybe_code { + deps.insert(code_dep.clone()); + } + if let Some(type_dep) = &dep.maybe_type { + deps.insert(type_dep.clone()); + } } - if let Some(type_dep) = &dep.maybe_type { - deps.insert(type_dep.clone()); + if let Some((_, types_dep)) = &module.maybe_types { + deps.insert(types_dep.clone()); } + let item = ModuleInfoMapItem { + deps: deps.into_iter().collect(), + size: module.size(), + }; + Some((specifier.clone(), item)) + } else { + None } - if let Some((_, types_dep)) = &module.maybe_types { - deps.insert(types_dep.clone()); - } - let item = ModuleInfoMapItem { - deps: deps.into_iter().collect(), - size: module.size(), - }; - (specifier.clone(), item) }) .collect(); ModuleInfoMap::new(map) } + /// Retrieve a map that contains a representation of each module in the graph + /// which can be used to provide code to a module loader without holding all + /// the state to be able to operate on the graph. + pub fn get_loadable_modules( + &self, + ) -> HashMap> { + let mut loadable_modules: HashMap< + ModuleSpecifier, + Result, + > = self.modules.iter().map(to_module_result).collect(); + for (specifier, _) in self.redirects.iter() { + if let Some(module_slot) = + self.modules.get(self.resolve_specifier(specifier)) + { + let (_, result) = to_module_result((specifier, module_slot)); + loadable_modules.insert(specifier.clone(), result); + } + } + loadable_modules + } + pub fn get_media_type( &self, specifier: &ModuleSpecifier, ) -> Option { - if let Some(module) = self.get_module(specifier) { + if let ModuleSlot::Module(module) = self.get_module(specifier) { Some(module.media_type) } else { None } } - fn get_module(&self, specifier: &ModuleSpecifier) -> Option<&Module> { + fn get_module(&self, specifier: &ModuleSpecifier) -> &ModuleSlot { let s = self.resolve_specifier(specifier); - self.modules.get(s) + if let Some(module_slot) = self.modules.get(s) { + module_slot + } else { + &ModuleSlot::None + } } fn get_module_mut( &mut self, specifier: &ModuleSpecifier, - ) -> Option<&mut Module> { + ) -> Option<&mut ModuleSlot> { // this is duplicated code because `.resolve_specifier` requires an // immutable borrow, but if `.resolve_specifier` is mut, then everything // that calls it is is mut @@ -1174,12 +1269,14 @@ impl Graph { // files will not get emitted. To counter act that behavior, we will // include all modules that are emittable. let mut specifiers = HashSet::<&ModuleSpecifier>::new(); - for (_, module) in self.modules.iter() { - if module.media_type == MediaType::JSX - || module.media_type == MediaType::TypeScript - || module.media_type == MediaType::TSX - { - specifiers.insert(&module.specifier); + for (_, module_slot) in self.modules.iter() { + if let ModuleSlot::Module(module) = module_slot { + if module.media_type == MediaType::JSX + || module.media_type == MediaType::TypeScript + || module.media_type == MediaType::TSX + { + specifiers.insert(&module.specifier); + } } } // We should include all the original roots as well. @@ -1196,7 +1293,12 @@ impl Graph { // if the root module has a types specifier, we should be sending that // to tsc instead of the original specifier let specifier = self.resolve_specifier(ms); - let module = self.get_module(specifier).unwrap(); + let module = + if let ModuleSlot::Module(module) = self.get_module(specifier) { + module + } else { + panic!("missing module"); + }; let specifier = if let Some((_, types_specifier)) = &module.maybe_types { self.resolve_specifier(types_specifier) @@ -1216,7 +1318,7 @@ impl Graph { /// Get the source for a given module specifier. If the module is not part /// of the graph, the result will be `None`. pub fn get_source(&self, specifier: &ModuleSpecifier) -> Option { - if let Some(module) = self.get_module(specifier) { + if let ModuleSlot::Module(module) = self.get_module(specifier) { Some(module.source.clone()) } else { None @@ -1232,7 +1334,11 @@ impl Graph { } let module = self.roots[0].clone(); - let m = self.get_module(&module).unwrap(); + let m = if let ModuleSlot::Module(module) = self.get_module(&module) { + module + } else { + return Err(GraphError::MissingSpecifier(module.clone()).into()); + }; let mut seen = HashSet::new(); let mut totals = HashMap::new(); @@ -1247,9 +1353,19 @@ impl Graph { (None, None) }; + let dep_count = self + .modules + .iter() + .filter_map(|(_, m)| match m { + ModuleSlot::Module(_) => Some(1), + _ => None, + }) + .count() + - 1; + Ok(ModuleGraphInfo { compiled, - dep_count: self.modules.len() - 1, + dep_count, file_type: m.media_type, files, info, @@ -1267,15 +1383,19 @@ impl Graph { let check_js = config.get_check_js(); let config = config.as_bytes(); self.modules.iter().all(|(_, m)| { - let needs_emit = match m.media_type { - MediaType::TypeScript | MediaType::TSX | MediaType::JSX => true, - MediaType::JavaScript => check_js, - _ => false, - }; - if needs_emit { - m.is_emit_valid(&config) + if let ModuleSlot::Module(m) = m { + let needs_emit = match m.media_type { + MediaType::TypeScript | MediaType::TSX | MediaType::JSX => true, + MediaType::JavaScript => check_js, + _ => false, + }; + if needs_emit { + m.is_emit_valid(&config) + } else { + true + } } else { - true + false } }) } @@ -1286,15 +1406,17 @@ impl Graph { pub fn lock(&self) { if let Some(lf) = self.maybe_lockfile.as_ref() { let mut lockfile = lf.lock().unwrap(); - for (ms, module) in self.modules.iter() { - let specifier = module.specifier.to_string(); - let valid = lockfile.check_or_insert(&specifier, &module.source); - if !valid { - eprintln!( - "{}", - GraphError::InvalidSource(ms.clone(), lockfile.filename.clone()) - ); - std::process::exit(10); + for (ms, module_slot) in self.modules.iter() { + if let ModuleSlot::Module(module) = module_slot { + let specifier = module.specifier.to_string(); + let valid = lockfile.check_or_insert(&specifier, &module.source); + if !valid { + eprintln!( + "{}", + GraphError::InvalidSource(ms.clone(), lockfile.filename.clone()) + ); + std::process::exit(10); + } } } } @@ -1305,9 +1427,12 @@ impl Graph { /// checked to determine if it is valid. fn needs_emit(&self, config: &TsConfig) -> bool { let check_js = config.get_check_js(); - self.modules.iter().any(|(_, m)| match m.media_type { - MediaType::TypeScript | MediaType::TSX | MediaType::JSX => true, - MediaType::JavaScript => check_js, + self.modules.iter().any(|(_, m)| match m { + ModuleSlot::Module(m) => match m.media_type { + MediaType::TypeScript | MediaType::TSX | MediaType::JSX => true, + MediaType::JavaScript => check_js, + _ => false, + }, _ => false, }) } @@ -1332,10 +1457,11 @@ impl Graph { referrer: &ModuleSpecifier, prefer_types: bool, ) -> Result { - if !self.contains_module(referrer) { - return Err(GraphError::MissingSpecifier(referrer.to_owned()).into()); - } - let module = self.get_module(referrer).unwrap(); + let module = if let ModuleSlot::Module(module) = self.get_module(referrer) { + module + } else { + return Err(GraphError::MissingSpecifier(referrer.clone()).into()); + }; if !module.dependencies.contains_key(specifier) { return Err( GraphError::MissingDependency( @@ -1363,7 +1489,11 @@ impl Graph { .into(), ); }; - if !self.contains_module(&resolved_specifier) { + let dep_module = if let ModuleSlot::Module(dep_module) = + self.get_module(&resolved_specifier) + { + dep_module + } else { return Err( GraphError::MissingDependency( referrer.to_owned(), @@ -1371,8 +1501,7 @@ impl Graph { ) .into(), ); - } - let dep_module = self.get_module(&resolved_specifier).unwrap(); + }; // In the case that there is a X-TypeScript-Types or a triple-slash types, // then the `maybe_types` specifier will be populated and we should use that // instead. @@ -1424,7 +1553,7 @@ impl Graph { pub fn transpile( &mut self, options: TranspileOptions, - ) -> Result<(Stats, Option), AnyError> { + ) -> Result { let start = Instant::now(); let mut ts_config = TsConfig::new(json!({ @@ -1443,37 +1572,39 @@ impl Graph { let mut emit_count: u128 = 0; let config = ts_config.as_bytes(); - for (_, module) in self.modules.iter_mut() { - // TODO(kitsonk) a lot of this logic should be refactored into `Module` as - // we start to support other methods on the graph. Especially managing - // the dirty state is something the module itself should "own". + for (_, module_slot) in self.modules.iter_mut() { + if let ModuleSlot::Module(module) = module_slot { + // TODO(kitsonk) a lot of this logic should be refactored into `Module` as + // we start to support other methods on the graph. Especially managing + // the dirty state is something the module itself should "own". - // if the module is a Dts file we should skip it - if module.media_type == MediaType::Dts { - continue; + // if the module is a Dts file we should skip it + if module.media_type == MediaType::Dts { + continue; + } + // if we don't have check_js enabled, we won't touch non TypeScript or JSX + // modules + if !(emit_options.check_js + || module.media_type == MediaType::JSX + || module.media_type == MediaType::TSX + || module.media_type == MediaType::TypeScript) + { + continue; + } + // skip modules that already have a valid emit + if !options.reload && module.is_emit_valid(&config) { + continue; + } + if module.maybe_parsed_module.is_none() { + module.parse()?; + } + let parsed_module = module.maybe_parsed_module.clone().unwrap(); + let emit = parsed_module.transpile(&emit_options)?; + emit_count += 1; + module.maybe_emit = Some(Emit::Cli(emit)); + module.set_version(&config); + module.is_dirty = true; } - // if we don't have check_js enabled, we won't touch non TypeScript or JSX - // modules - if !(emit_options.check_js - || module.media_type == MediaType::JSX - || module.media_type == MediaType::TSX - || module.media_type == MediaType::TypeScript) - { - continue; - } - // skip modules that already have a valid emit - if !options.reload && module.is_emit_valid(&config) { - continue; - } - if module.maybe_parsed_module.is_none() { - module.parse()?; - } - let parsed_module = module.maybe_parsed_module.clone().unwrap(); - let emit = parsed_module.transpile(&emit_options)?; - emit_count += 1; - module.maybe_emit = Some(Emit::Cli(emit)); - module.set_version(&config); - module.is_dirty = true; } self.flush()?; @@ -1483,7 +1614,12 @@ impl Graph { ("Total time".to_string(), start.elapsed().as_millis()), ]); - Ok((stats, maybe_ignored_options)) + Ok(ResultInfo { + diagnostics: Default::default(), + loadable_modules: self.get_loadable_modules(), + maybe_ignored_options, + stats, + }) } } @@ -1510,7 +1646,6 @@ impl swc_bundler::Resolve for Graph { /// A structure for building a dependency graph of modules. pub struct GraphBuilder { - fetched: HashSet, graph: Graph, maybe_import_map: Option>>, pending: FuturesUnordered, @@ -1529,7 +1664,6 @@ impl GraphBuilder { }; GraphBuilder { graph: Graph::new(handler, maybe_lockfile), - fetched: HashSet::new(), maybe_import_map: internal_import_map, pending: FuturesUnordered::new(), } @@ -1543,12 +1677,22 @@ impl GraphBuilder { specifier: &ModuleSpecifier, is_dynamic: bool, ) -> Result<(), AnyError> { - self.fetch(specifier, &None, is_dynamic)?; + self.fetch(specifier, &None, is_dynamic); loop { - let cached_module = self.pending.next().await.unwrap()?; - let is_root = &cached_module.specifier == specifier; - self.visit(cached_module, is_root)?; + match self.pending.next().await { + Some(Err((specifier, err))) => { + self + .graph + .modules + .insert(specifier, ModuleSlot::Err(Rc::new(err))); + } + Some(Ok(cached_module)) => { + let is_root = &cached_module.specifier == specifier; + self.visit(cached_module, is_root)?; + } + _ => {} + } if self.pending.is_empty() { break; } @@ -1573,20 +1717,19 @@ impl GraphBuilder { specifier: &ModuleSpecifier, maybe_referrer: &Option, is_dynamic: bool, - ) -> Result<(), AnyError> { - if self.fetched.contains(&specifier) { - return Ok(()); + ) { + if !self.graph.modules.contains_key(&specifier) { + self + .graph + .modules + .insert(specifier.clone(), ModuleSlot::Pending); + let future = self.graph.handler.borrow_mut().fetch( + specifier.clone(), + maybe_referrer.clone(), + is_dynamic, + ); + self.pending.push(future); } - - self.fetched.insert(specifier.clone()); - let future = self.graph.handler.borrow_mut().fetch( - specifier.clone(), - maybe_referrer.clone(), - is_dynamic, - ); - self.pending.push(future); - - Ok(()) } /// Visit a module that has been fetched, hydrating the module, analyzing its @@ -1632,14 +1775,14 @@ impl GraphBuilder { for (_, dep) in module.dependencies.iter() { let maybe_referrer = Some(dep.location.clone()); if let Some(specifier) = dep.maybe_code.as_ref() { - self.fetch(specifier, &maybe_referrer, dep.is_dynamic)?; + self.fetch(specifier, &maybe_referrer, dep.is_dynamic); } if let Some(specifier) = dep.maybe_type.as_ref() { - self.fetch(specifier, &maybe_referrer, dep.is_dynamic)?; + self.fetch(specifier, &maybe_referrer, dep.is_dynamic); } } if let Some((_, specifier)) = module.maybe_types.as_ref() { - self.fetch(specifier, &None, false)?; + self.fetch(specifier, &None, false); } if specifier != requested_specifier { self @@ -1647,7 +1790,10 @@ impl GraphBuilder { .redirects .insert(requested_specifier, specifier.clone()); } - self.graph.modules.insert(specifier, module); + self + .graph + .modules + .insert(specifier, ModuleSlot::Module(Box::new(module))); Ok(()) } @@ -1702,7 +1848,7 @@ pub mod tests { fn get_cache( &self, specifier: ModuleSpecifier, - ) -> Result { + ) -> Result { let specifier_text = specifier .to_string() .replace(":///", "_") @@ -1710,7 +1856,8 @@ pub mod tests { .replace("/", "-"); let source_path = self.fixtures.join(specifier_text); let media_type = MediaType::from(&source_path); - let source = fs::read_to_string(&source_path)?; + let source = fs::read_to_string(&source_path) + .map_err(|err| (specifier.clone(), err.into()))?; let is_remote = specifier.as_url().scheme() != "file"; Ok(CachedModule { @@ -2280,10 +2427,9 @@ pub mod tests { ModuleSpecifier::resolve_url_or_path("file:///tests/main.ts") .expect("could not resolve module"); let (mut graph, handler) = setup(specifier).await; - let (stats, maybe_ignored_options) = - graph.transpile(TranspileOptions::default()).unwrap(); - assert_eq!(stats.0.len(), 3); - assert_eq!(maybe_ignored_options, None); + let result_info = graph.transpile(TranspileOptions::default()).unwrap(); + assert_eq!(result_info.stats.0.len(), 3); + assert_eq!(result_info.maybe_ignored_options, None); let h = handler.borrow(); assert_eq!(h.cache_calls.len(), 2); match &h.cache_calls[0].1 { @@ -2334,7 +2480,7 @@ pub mod tests { ModuleSpecifier::resolve_url_or_path("https://deno.land/x/transpile.tsx") .expect("could not resolve module"); let (mut graph, handler) = setup(specifier).await; - let (_, maybe_ignored_options) = graph + let result_info = graph .transpile(TranspileOptions { debug: false, maybe_config_path: Some("tests/module_graph/tsconfig.json".to_string()), @@ -2342,7 +2488,7 @@ pub mod tests { }) .unwrap(); assert_eq!( - maybe_ignored_options.unwrap().items, + result_info.maybe_ignored_options.unwrap().items, vec!["target".to_string()], "the 'target' options should have been ignored" ); diff --git a/cli/module_loader.rs b/cli/module_loader.rs index da75b8510a..aab951c4a5 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -94,26 +94,14 @@ impl ModuleLoader for CliModuleLoader { maybe_referrer: Option, _is_dynamic: bool, ) -> Pin> { - let module_specifier = module_specifier.to_owned(); - let module_url_specified = module_specifier.to_string(); + let module_specifier = module_specifier.clone(); let program_state = self.program_state.clone(); // NOTE: this block is async only because of `deno_core` // interface requirements; module was already loaded // when constructing module graph during call to `prepare_load`. - let fut = async move { - let compiled_module = program_state - .fetch_compiled_module(module_specifier, maybe_referrer)?; - Ok(deno_core::ModuleSource { - // Real module name, might be different from initial specifier - // due to redirections. - code: compiled_module.code, - module_url_specified, - module_url_found: compiled_module.name, - }) - }; - - fut.boxed_local() + async move { program_state.load(module_specifier, maybe_referrer) } + .boxed_local() } fn prepare_load( diff --git a/cli/program_state.rs b/cli/program_state.rs index 223d043bae..008244b5f6 100644 --- a/cli/program_state.rs +++ b/cli/program_state.rs @@ -8,7 +8,6 @@ use crate::http_cache; use crate::http_util; use crate::import_map::ImportMap; use crate::lockfile::Lockfile; -use crate::media_type::MediaType; use crate::module_graph::CheckOptions; use crate::module_graph::GraphBuilder; use crate::module_graph::TranspileOptions; @@ -18,11 +17,14 @@ use crate::specifier_handler::FetchHandler; use deno_runtime::inspector::InspectorServer; use deno_runtime::permissions::Permissions; -use deno_core::error::generic_error; +use deno_core::error::anyhow; +use deno_core::error::get_custom_error_class; use deno_core::error::AnyError; use deno_core::url::Url; +use deno_core::ModuleSource; use deno_core::ModuleSpecifier; use std::cell::RefCell; +use std::collections::HashMap; use std::env; use std::rc::Rc; use std::sync::Arc; @@ -36,12 +38,6 @@ pub fn exit_unstable(api_name: &str) { std::process::exit(70); } -// TODO(@kitsonk) probably can refactor this better with the graph. -pub struct CompiledModule { - pub code: String, - pub name: String, -} - /// This structure represents state of single "deno" program. /// /// It is shared by all created workers (thus V8 isolates). @@ -50,6 +46,8 @@ pub struct ProgramState { pub flags: flags::Flags, pub dir: deno_dir::DenoDir, pub file_fetcher: FileFetcher, + pub modules: + Arc>>>, pub lockfile: Option>>, pub maybe_import_map: Option, pub maybe_inspector_server: Option>, @@ -111,6 +109,7 @@ impl ProgramState { dir, flags, file_fetcher, + modules: Default::default(), lockfile, maybe_import_map, maybe_inspector_server, @@ -146,17 +145,17 @@ impl ProgramState { let debug = self.flags.log_level == Some(log::Level::Debug); let maybe_config_path = self.flags.config_path.clone(); - if self.flags.no_check { - let (stats, maybe_ignored_options) = - graph.transpile(TranspileOptions { - debug, - maybe_config_path, - reload: self.flags.reload, - })?; - debug!("{}", stats); - if let Some(ignored_options) = maybe_ignored_options { - eprintln!("{}", ignored_options); + let result_modules = if self.flags.no_check { + let result_info = graph.transpile(TranspileOptions { + debug, + maybe_config_path, + reload: self.flags.reload, + })?; + debug!("{}", result_info.stats); + if let Some(ignored_options) = result_info.maybe_ignored_options { + warn!("{}", ignored_options); } + result_info.loadable_modules } else { let result_info = graph.check(CheckOptions { debug, @@ -171,10 +170,14 @@ impl ProgramState { eprintln!("{}", ignored_options); } if !result_info.diagnostics.is_empty() { - return Err(generic_error(result_info.diagnostics.to_string())); + return Err(anyhow!(result_info.diagnostics)); } + result_info.loadable_modules }; + let mut loadable_modules = self.modules.lock().unwrap(); + loadable_modules.extend(result_modules); + if let Some(ref lockfile) = self.lockfile { let g = lockfile.lock().unwrap(); g.write()?; @@ -183,56 +186,55 @@ impl ProgramState { Ok(()) } - pub fn fetch_compiled_module( + pub fn load( &self, - module_specifier: ModuleSpecifier, + specifier: ModuleSpecifier, maybe_referrer: Option, - ) -> Result { - // TODO(@kitsonk) this really needs to be avoided and refactored out, as we - // really should just be getting this from the module graph. - let out = self - .file_fetcher - .get_source(&module_specifier) - .expect("Cached source file doesn't exist"); - - let specifier = out.specifier.clone(); - let compiled_module = if let Some((code, _)) = - self.get_emit(&specifier.as_url()) - { - CompiledModule { - code: String::from_utf8(code).unwrap(), - name: specifier.as_url().to_string(), - } - // We expect a compiled source for any non-JavaScript files, except for - // local files that have an unknown media type and no referrer (root modules - // that do not have an extension.) - } else if out.media_type != MediaType::JavaScript - && !(out.media_type == MediaType::Unknown - && maybe_referrer.is_none() - && specifier.as_url().scheme() == "file") - { - let message = if let Some(referrer) = maybe_referrer { - format!("Compiled module not found \"{}\"\n From: {}\n If the source module contains only types, use `import type` and `export type` to import it instead.", module_specifier, referrer) - } else { - format!("Compiled module not found \"{}\"\n If the source module contains only types, use `import type` and `export type` to import it instead.", module_specifier) - }; - info!("{}: {}", crate::colors::yellow("warning"), message); - CompiledModule { - code: "".to_string(), - name: specifier.as_url().to_string(), - } - } else { - CompiledModule { - code: out.source, - name: specifier.as_url().to_string(), - } - }; - - Ok(compiled_module) + ) -> Result { + let modules = self.modules.lock().unwrap(); + modules + .get(&specifier) + .map(|r| match r { + Ok(module_source) => Ok(module_source.clone()), + Err(err) => { + // TODO(@kitsonk) this feels a bit hacky but it works, without + // introducing another enum to have to try to deal with. + if get_custom_error_class(err) == Some("NotFound") { + let message = if let Some(referrer) = &maybe_referrer { + format!("{}\n From: {}\n If the source module contains only types, use `import type` and `export type` to import it instead.", err, referrer) + } else { + format!("{}\n If the source module contains only types, use `import type` and `export type` to import it instead.", err) + }; + warn!("{}: {}", crate::colors::yellow("warning"), message); + Ok(ModuleSource { + code: "".to_string(), + module_url_found: specifier.to_string(), + module_url_specified: specifier.to_string(), + }) + } else { + // anyhow errors don't support cloning, so we have to manage this + // ourselves + Err(anyhow!(err.to_string())) + } + }, + }) + .unwrap_or_else(|| { + if let Some(referrer) = maybe_referrer { + Err(anyhow!( + "Module \"{}\" is missing from the graph.\n From: {}", + specifier, + referrer + )) + } else { + Err(anyhow!( + "Module \"{}\" is missing from the graph.", + specifier + )) + } + }) } - // TODO(@kitsonk) this should be a straight forward API on file_fetcher or - // whatever future refactors do... + // TODO(@kitsonk) this should be refactored to get it from the module graph fn get_emit(&self, url: &Url) -> Option<(Vec, Option>)> { match url.scheme() { // we should only be looking for emits for schemes that denote external @@ -245,11 +247,11 @@ impl ProgramState { let emit_path = self .dir .gen_cache - .get_cache_filename_with_extension(&url, "js"); + .get_cache_filename_with_extension(&url, "js")?; let emit_map_path = self .dir .gen_cache - .get_cache_filename_with_extension(&url, "js.map"); + .get_cache_filename_with_extension(&url, "js.map")?; if let Ok(code) = self.dir.gen_cache.get(&emit_path) { let maybe_map = if let Ok(map) = self.dir.gen_cache.get(&emit_map_path) { Some(map) diff --git a/cli/specifier_handler.rs b/cli/specifier_handler.rs index 02a1196d3a..7523164675 100644 --- a/cli/specifier_handler.rs +++ b/cli/specifier_handler.rs @@ -25,8 +25,12 @@ use std::pin::Pin; use std::sync::Arc; pub type DependencyMap = HashMap; -pub type FetchFuture = - Pin> + 'static)>>; +pub type FetchFuture = Pin< + Box< + (dyn Future> + + 'static), + >, +>; /// A group of errors that represent errors that can occur with an /// an implementation of `SpecifierHandler`. @@ -287,23 +291,30 @@ impl SpecifierHandler for FetchHandler { // they cannot actually get to the source code that is quoted, as // it only exists in the runtime memory of Deno. if !location.filename.contains("$deno$") { - HandlerError::FetchErrorWithLocation(err.to_string(), location) - .into() + ( + requested_specifier.clone(), + HandlerError::FetchErrorWithLocation(err.to_string(), location) + .into(), + ) } else { - err + (requested_specifier.clone(), err) } } else { - err + (requested_specifier.clone(), err) } })?; let url = source_file.specifier.as_url(); let is_remote = url.scheme() != "file"; let filename = disk_cache.get_cache_filename_with_extension(url, "meta"); - let maybe_version = if let Ok(bytes) = disk_cache.get(&filename) { - if let Ok(compiled_file_metadata) = - CompiledFileMetadata::from_bytes(&bytes) - { - Some(compiled_file_metadata.version_hash) + let maybe_version = if let Some(filename) = filename { + if let Ok(bytes) = disk_cache.get(&filename) { + if let Ok(compiled_file_metadata) = + CompiledFileMetadata::from_bytes(&bytes) + { + Some(compiled_file_metadata.version_hash) + } else { + None + } } else { None } @@ -314,19 +325,26 @@ impl SpecifierHandler for FetchHandler { let mut maybe_map_path = None; let map_path = disk_cache.get_cache_filename_with_extension(&url, "js.map"); - let maybe_map = if let Ok(map) = disk_cache.get(&map_path) { - maybe_map_path = Some(disk_cache.location.join(map_path)); - Some(String::from_utf8(map)?) + let maybe_map = if let Some(map_path) = map_path { + if let Ok(map) = disk_cache.get(&map_path) { + maybe_map_path = Some(disk_cache.location.join(map_path)); + Some(String::from_utf8(map).unwrap()) + } else { + None + } } else { None }; let mut maybe_emit = None; let mut maybe_emit_path = None; let emit_path = disk_cache.get_cache_filename_with_extension(&url, "js"); - if let Ok(code) = disk_cache.get(&emit_path) { - maybe_emit = Some(Emit::Cli((String::from_utf8(code)?, maybe_map))); - maybe_emit_path = - Some((disk_cache.location.join(emit_path), maybe_map_path)); + if let Some(emit_path) = emit_path { + if let Ok(code) = disk_cache.get(&emit_path) { + maybe_emit = + Some(Emit::Cli((String::from_utf8(code).unwrap(), maybe_map))); + maybe_emit_path = + Some((disk_cache.location.join(emit_path), maybe_map_path)); + } }; Ok(CachedModule { @@ -353,8 +371,12 @@ impl SpecifierHandler for FetchHandler { let filename = self .disk_cache .get_cache_filename_with_extension(specifier.as_url(), "buildinfo"); - if let Ok(tsbuildinfo) = self.disk_cache.get(&filename) { - Ok(Some(String::from_utf8(tsbuildinfo)?)) + if let Some(filename) = filename { + if let Ok(tsbuildinfo) = self.disk_cache.get(&filename) { + Ok(Some(String::from_utf8(tsbuildinfo)?)) + } else { + Ok(None) + } } else { Ok(None) } @@ -367,7 +389,8 @@ impl SpecifierHandler for FetchHandler { ) -> Result<(), AnyError> { let filename = self .disk_cache - .get_cache_filename_with_extension(specifier.as_url(), "buildinfo"); + .get_cache_filename_with_extension(specifier.as_url(), "buildinfo") + .unwrap(); debug!("set_tsbuildinfo - filename {:?}", filename); self .disk_cache @@ -383,14 +406,17 @@ impl SpecifierHandler for FetchHandler { match emit { Emit::Cli((code, maybe_map)) => { let url = specifier.as_url(); - let filename = - self.disk_cache.get_cache_filename_with_extension(url, "js"); + let filename = self + .disk_cache + .get_cache_filename_with_extension(url, "js") + .unwrap(); self.disk_cache.set(&filename, code.as_bytes())?; if let Some(map) = maybe_map { let filename = self .disk_cache - .get_cache_filename_with_extension(url, "js.map"); + .get_cache_filename_with_extension(url, "js.map") + .unwrap(); self.disk_cache.set(&filename, map.as_bytes())?; } } @@ -425,7 +451,8 @@ impl SpecifierHandler for FetchHandler { let compiled_file_metadata = CompiledFileMetadata { version_hash }; let filename = self .disk_cache - .get_cache_filename_with_extension(specifier.as_url(), "meta"); + .get_cache_filename_with_extension(specifier.as_url(), "meta") + .unwrap(); self .disk_cache @@ -475,9 +502,12 @@ impl SpecifierHandler for MemoryHandler { ..Default::default() }) } else { - Err(custom_error( - "NotFound", - format!("Unable to find specifier in sources: {}", specifier), + Err(( + specifier.clone(), + custom_error( + "NotFound", + format!("Unable to find specifier in sources: {}", specifier), + ), )) }; diff --git a/cli/tests/dynamic_import/b.js b/cli/tests/dynamic_import/b.js new file mode 100644 index 0000000000..6ea50d3608 --- /dev/null +++ b/cli/tests/dynamic_import/b.js @@ -0,0 +1,2 @@ +import "./bad.mjs"; +export default () => "error"; diff --git a/cli/tests/dynamic_import/c.js b/cli/tests/dynamic_import/c.js new file mode 100644 index 0000000000..20546455ea --- /dev/null +++ b/cli/tests/dynamic_import/c.js @@ -0,0 +1,2 @@ +await import("./bad2.mjs"); +export default () => "error"; diff --git a/cli/tests/error_004_missing_module.ts b/cli/tests/error_004_missing_module.ts index 24ae52cf72..ab5350408c 100644 --- a/cli/tests/error_004_missing_module.ts +++ b/cli/tests/error_004_missing_module.ts @@ -1,2 +1,4 @@ // eslint-disable-next-line import * as badModule from "./bad-module.ts"; + +console.log(badModule); diff --git a/cli/tests/error_005_missing_dynamic_import.ts.out b/cli/tests/error_005_missing_dynamic_import.ts.out index 79c61e7c89..bffa907961 100644 --- a/cli/tests/error_005_missing_dynamic_import.ts.out +++ b/cli/tests/error_005_missing_dynamic_import.ts.out @@ -1,2 +1 @@ -error: Cannot resolve module "[WILDCARD]/bad-module.ts" from "[WILDCARD]/error_005_missing_dynamic_import.ts". - at file:///[WILDCARD]/error_005_missing_dynamic_import.ts:3:26 +error: Uncaught (in promise) TypeError: Cannot resolve module "[WILDCARD]/cli/tests/bad-module.ts". diff --git a/cli/tests/error_015_dynamic_import_permissions.out b/cli/tests/error_015_dynamic_import_permissions.out index b385da0d45..577dbcc5ce 100644 --- a/cli/tests/error_015_dynamic_import_permissions.out +++ b/cli/tests/error_015_dynamic_import_permissions.out @@ -1,2 +1 @@ -error: network access to "http://localhost:4545/cli/tests/subdir/mod4.js", run again with the --allow-net flag - at file:///[WILDCARD]cli/tests/error_015_dynamic_import_permissions.js:[WILDCARD] +error: Uncaught (in promise) TypeError: network access to "http://localhost:4545/cli/tests/subdir/mod4.js", run again with the --allow-net flag diff --git a/cli/tests/fix_dynamic_import_errors.js b/cli/tests/fix_dynamic_import_errors.js new file mode 100644 index 0000000000..317047ccb5 --- /dev/null +++ b/cli/tests/fix_dynamic_import_errors.js @@ -0,0 +1,7 @@ +import("./dynamic_import/b.js").catch(() => { + console.log("caught import error from b.js"); +}); + +import("./dynamic_import/c.js").catch(() => { + console.log("caught import error from c.js"); +}); diff --git a/cli/tests/fix_dynamic_import_errors.js.out b/cli/tests/fix_dynamic_import_errors.js.out new file mode 100644 index 0000000000..e7856fb9ce --- /dev/null +++ b/cli/tests/fix_dynamic_import_errors.js.out @@ -0,0 +1,2 @@ +caught import error from [WILDCARD].js +caught import error from [WILDCARD].js diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index e4de423ee0..2f23e2dee6 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -3227,6 +3227,11 @@ itest!(tsx_imports { output: "tsx_imports.ts.out", }); +itest!(fix_dynamic_import_errors { + args: "run --reload fix_dynamic_import_errors.js", + output: "fix_dynamic_import_errors.js.out", +}); + itest!(fix_emittable_skipped { args: "run --reload fix_emittable_skipped.js", output: "fix_emittable_skipped.ts.out", diff --git a/cli/tests/redirect_cache.out b/cli/tests/redirect_cache.out index ad26d01086..609e523f7f 100644 --- a/cli/tests/redirect_cache.out +++ b/cli/tests/redirect_cache.out @@ -2,5 +2,4 @@ Download http://localhost:4548/cli/tests/subdir/redirects/a.ts Download http://localhost:4546/cli/tests/subdir/redirects/a.ts Download http://localhost:4545/cli/tests/subdir/redirects/a.ts Download http://localhost:4545/cli/tests/subdir/redirects/b.ts -Download http://localhost:4545/cli/tests/subdir/redirects/a.ts Check http://localhost:4548/cli/tests/subdir/redirects/a.ts diff --git a/cli/tests/unsupported_dynamic_import_scheme.out b/cli/tests/unsupported_dynamic_import_scheme.out index 0161b7a99f..434f14c4c1 100644 --- a/cli/tests/unsupported_dynamic_import_scheme.out +++ b/cli/tests/unsupported_dynamic_import_scheme.out @@ -1,4 +1,4 @@ -error: Unsupported scheme "xxx" for module "xxx:". Supported schemes: [ +error: Uncaught (in promise) TypeError: Unsupported scheme "xxx" for module "xxx:". Supported schemes: [ "http", "https", "file", diff --git a/core/modules.rs b/core/modules.rs index 6f330f5594..546f2464f8 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -43,7 +43,7 @@ pub type ModuleLoadId = i32; // that happened; not only first and final target. It would simplify a lot // of things throughout the codebase otherwise we may end up requesting // intermediate redirects from file loader. -#[derive(Debug, Eq, PartialEq)] +#[derive(Debug, Clone, Eq, PartialEq)] pub struct ModuleSource { pub code: String, pub module_url_specified: String,