From cfe4369dedcdd885c60b8a5f6c89bc4bb7e5e1e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Mon, 2 Mar 2020 19:12:49 +0100 Subject: [PATCH] refactor: rename structures related to Modules (#4217) * rename structures related to ES Modules; add "Modules" prefix * remove unneeded Unpin trait requirement for "ModuleLoader" --- cli/ops/compiler.rs | 2 +- cli/state.rs | 8 +++---- core/es_isolate.rs | 47 +++++++++++++++------------------------ core/modules.rs | 54 +++++++++++++++++++++++++++++++-------------- 4 files changed, 60 insertions(+), 51 deletions(-) diff --git a/cli/ops/compiler.rs b/cli/ops/compiler.rs index 9e889551..e6ed364d 100644 --- a/cli/ops/compiler.rs +++ b/cli/ops/compiler.rs @@ -6,7 +6,7 @@ use crate::futures::future::try_join_all; use crate::msg; use crate::op_error::OpError; use crate::state::State; -use deno_core::Loader; +use deno_core::ModuleLoader; use deno_core::*; use futures::future::FutureExt; diff --git a/cli/state.rs b/cli/state.rs index 5b841609..8ad12203 100644 --- a/cli/state.rs +++ b/cli/state.rs @@ -12,7 +12,7 @@ use crate::worker::WorkerHandle; use deno_core::Buf; use deno_core::CoreOp; use deno_core::ErrBox; -use deno_core::Loader; +use deno_core::ModuleLoader; use deno_core::ModuleSpecifier; use deno_core::Op; use deno_core::ResourceTable; @@ -151,7 +151,7 @@ impl State { } } -impl Loader for State { +impl ModuleLoader for State { fn resolve( &self, specifier: &str, @@ -178,7 +178,7 @@ impl Loader for State { module_specifier: &ModuleSpecifier, maybe_referrer: Option, is_dyn_import: bool, - ) -> Pin> { + ) -> Pin> { let module_specifier = module_specifier.clone(); if is_dyn_import { if let Err(e) = self.check_dyn_import(&module_specifier) { @@ -196,7 +196,7 @@ impl Loader for State { let compiled_module = global_state .fetch_compiled_module(module_specifier, maybe_referrer, target_lib) .await?; - Ok(deno_core::SourceCodeInfo { + Ok(deno_core::ModuleSource { // Real module name, might be different from initial specifier // due to redirections. code: compiled_module.code, diff --git a/core/es_isolate.rs b/core/es_isolate.rs index 1147741d..e0e414bc 100644 --- a/core/es_isolate.rs +++ b/core/es_isolate.rs @@ -31,34 +31,23 @@ use crate::isolate::Isolate; use crate::isolate::StartupData; use crate::module_specifier::ModuleSpecifier; use crate::modules::LoadState; -use crate::modules::Loader; +use crate::modules::ModuleLoader; +use crate::modules::ModuleSource; use crate::modules::Modules; use crate::modules::RecursiveModuleLoad; pub type ModuleId = i32; pub type DynImportId = i32; -/// Represent result of fetching the source code of a module. Found module URL -/// might be different from specified URL used for loading due to redirections -/// (like HTTP 303). E.G. Both https://example.com/a.ts and -/// https://example.com/b.ts may point to https://example.com/c.ts -/// By keeping track of specified and found URL we can alias modules and avoid -/// recompiling the same code 3 times. -#[derive(Debug, Eq, PartialEq)] -pub struct SourceCodeInfo { - pub code: String, - pub module_url_specified: String, - pub module_url_found: String, -} /// More specialized version of `Isolate` that provides loading /// and execution of ES Modules. /// /// Creating `EsIsolate` requires to pass `loader` argument -/// that implements `Loader` trait - that way actual resolution and +/// that implements `ModuleLoader` trait - that way actual resolution and /// loading of modules can be customized by the implementor. pub struct EsIsolate { core_isolate: Box, - loader: Rc, + loader: Rc, pub modules: Modules, pub(crate) next_dyn_import_id: DynImportId, pub(crate) dyn_import_map: @@ -84,7 +73,7 @@ impl DerefMut for EsIsolate { impl EsIsolate { pub fn new( - loader: Rc, + loader: Rc, startup_data: StartupData, will_snapshot: bool, ) -> Box { @@ -429,10 +418,10 @@ impl EsIsolate { fn register_during_load( &mut self, - info: SourceCodeInfo, + info: ModuleSource, load: &mut RecursiveModuleLoad, ) -> Result<(), ErrBox> { - let SourceCodeInfo { + let ModuleSource { code, module_url_specified, module_url_found, @@ -553,7 +542,7 @@ pub mod tests { use crate::isolate::js_check; use crate::isolate::tests::run_in_task; use crate::isolate::ZeroCopyBuf; - use crate::modules::SourceCodeInfoFuture; + use crate::modules::ModuleSourceFuture; use crate::ops::*; use std::io; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -566,7 +555,7 @@ pub mod tests { pub count: Arc, } - impl Loader for ModsLoader { + impl ModuleLoader for ModsLoader { fn resolve( &self, specifier: &str, @@ -585,7 +574,7 @@ pub mod tests { _module_specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, - ) -> Pin> { + ) -> Pin> { unreachable!() } } @@ -665,7 +654,7 @@ pub mod tests { pub count: Arc, } - impl Loader for DynImportErrLoader { + impl ModuleLoader for DynImportErrLoader { fn resolve( &self, specifier: &str, @@ -684,7 +673,7 @@ pub mod tests { _module_specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, - ) -> Pin> { + ) -> Pin> { async { Err(ErrBox::from(io::Error::from(io::ErrorKind::NotFound))) } .boxed() } @@ -726,7 +715,7 @@ pub mod tests { pub count: Arc, } - impl Loader for DynImportErr2Loader { + impl ModuleLoader for DynImportErr2Loader { fn resolve( &self, specifier: &str, @@ -750,8 +739,8 @@ pub mod tests { &self, specifier: &ModuleSpecifier, _maybe_referrer: Option, - ) -> Pin> { - let info = SourceCodeInfo { + ) -> Pin> { + let info = ModuleSource { module_url_specified: specifier.to_string(), module_url_found: specifier.to_string(), code: "# not valid JS".to_owned(), @@ -811,7 +800,7 @@ pub mod tests { pub load_count: Arc, } - impl Loader for DynImportOkLoader { + impl ModuleLoader for DynImportOkLoader { fn resolve( &self, specifier: &str, @@ -834,9 +823,9 @@ pub mod tests { specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, - ) -> Pin> { + ) -> Pin> { self.load_count.fetch_add(1, Ordering::Relaxed); - let info = SourceCodeInfo { + let info = ModuleSource { module_url_specified: specifier.to_string(), module_url_found: specifier.to_string(), code: "export function b() { return 'b' }".to_owned(), diff --git a/core/modules.rs b/core/modules.rs index 764aadb6..fbdf21b8 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -5,7 +5,6 @@ use rusty_v8 as v8; use crate::any_error::ErrBox; use crate::es_isolate::DynImportId; use crate::es_isolate::ModuleId; -use crate::es_isolate::SourceCodeInfo; use crate::module_specifier::ModuleSpecifier; use futures::future::FutureExt; use futures::stream::FuturesUnordered; @@ -20,10 +19,31 @@ use std::rc::Rc; use std::task::Context; use std::task::Poll; -pub type SourceCodeInfoFuture = - dyn Future>; +/// EsModule source code that will be loaded into V8. +/// +/// Users can implement `Into` for different file types that +/// can be transpiled to valid EsModule. +/// +/// Found module URL might be different from specified URL +/// used for loading due to redirections (like HTTP 303). +/// Eg. Both "https://example.com/a.ts" and +/// "https://example.com/b.ts" may point to "https://example.com/c.ts" +/// By keeping track of specified and found URL we can alias modules and avoid +/// recompiling the same code 3 times. +// TODO(bartlomieju): I have a strong opinion we should store all redirects +// 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)] +pub struct ModuleSource { + pub code: String, + pub module_url_specified: String, + pub module_url_found: String, +} + +pub type ModuleSourceFuture = dyn Future>; -pub trait Loader { +pub trait ModuleLoader { /// Returns an absolute URL. /// When implementing an spec-complaint VM, this should be exactly the /// algorithm described here: @@ -47,7 +67,7 @@ pub trait Loader { module_specifier: &ModuleSpecifier, maybe_referrer: Option, is_dyn_import: bool, - ) -> Pin>; + ) -> Pin>; } #[derive(Debug, Eq, PartialEq)] @@ -74,8 +94,8 @@ pub struct RecursiveModuleLoad { // Kind::Main pub dyn_import_id: Option, pub state: LoadState, - pub loader: Rc, - pub pending: FuturesUnordered>>, + pub loader: Rc, + pub pending: FuturesUnordered>>, pub is_pending: HashSet, } @@ -84,7 +104,7 @@ impl RecursiveModuleLoad { pub fn main( specifier: &str, code: Option, - loader: Rc, + loader: Rc, ) -> Self { let kind = Kind::Main; let state = LoadState::ResolveMain(specifier.to_owned(), code); @@ -95,7 +115,7 @@ impl RecursiveModuleLoad { id: DynImportId, specifier: &str, referrer: &str, - loader: Rc, + loader: Rc, ) -> Self { let kind = Kind::DynamicImport; let state = @@ -110,7 +130,7 @@ impl RecursiveModuleLoad { fn new( kind: Kind, state: LoadState, - loader: Rc, + loader: Rc, dyn_import_id: Option, ) -> Self { Self { @@ -138,7 +158,7 @@ impl RecursiveModuleLoad { let load_fut = match &self.state { LoadState::ResolveMain(_, Some(code)) => { - futures::future::ok(SourceCodeInfo { + futures::future::ok(ModuleSource { code: code.to_owned(), module_url_specified: module_specifier.to_string(), module_url_found: module_specifier.to_string(), @@ -174,7 +194,7 @@ impl RecursiveModuleLoad { } impl Stream for RecursiveModuleLoad { - type Item = Result; + type Item = Result; fn poll_next( self: Pin<&mut Self>, @@ -544,7 +564,7 @@ mod tests { } impl Future for DelayedSourceCodeFuture { - type Output = Result; + type Output = Result; fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll { let inner = self.get_mut(); @@ -559,7 +579,7 @@ mod tests { return Poll::Pending; } match mock_source_code(&inner.url) { - Some(src) => Poll::Ready(Ok(SourceCodeInfo { + Some(src) => Poll::Ready(Ok(ModuleSource { code: src.0.to_owned(), module_url_specified: inner.url.clone(), module_url_found: src.1.to_owned(), @@ -569,7 +589,7 @@ mod tests { } } - impl Loader for MockLoader { + impl ModuleLoader for MockLoader { fn resolve( &self, specifier: &str, @@ -602,7 +622,7 @@ mod tests { module_specifier: &ModuleSpecifier, _maybe_referrer: Option, _is_dyn_import: bool, - ) -> Pin> { + ) -> Pin> { let mut loads = self.loads.lock().unwrap(); loads.push(module_specifier.to_string()); let url = module_specifier.to_string(); @@ -835,7 +855,7 @@ mod tests { // slow.js const SLOW_SRC: &str = r#" // Circular import of never_ready.js - // Does this trigger two Loader calls? It shouldn't. + // Does this trigger two ModuleLoader calls? It shouldn't. import "/never_ready.js"; import "/a.js"; "#; -- GitLab