Skip to content

Commit

Permalink
refactor: rename structures related to Modules (denoland#4217)
Browse files Browse the repository at this point in the history 
* rename structures related to ES Modules; add "Modules" prefix
* remove unneeded Unpin trait requirement for "ModuleLoader"
  • Loading branch information
@bartlomieju
bartlomieju committed Mar 2, 2020
1 parent 4a47ffa commit cfe4369
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 51 deletions.
2 changes: 1 addition & 1 deletion cli/ops/compiler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
8 changes: 4 additions & 4 deletions cli/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -151,7 +151,7 @@ impl State {
}
}

impl Loader for State {
impl ModuleLoader for State {
fn resolve(
&self,
specifier: &str,
Expand All @@ -178,7 +178,7 @@ impl Loader for State {
module_specifier: &ModuleSpecifier,
maybe_referrer: Option<ModuleSpecifier>,
is_dyn_import: bool,
) -> Pin<Box<deno_core::SourceCodeInfoFuture>> {
) -> Pin<Box<deno_core::ModuleSourceFuture>> {
let module_specifier = module_specifier.clone();
if is_dyn_import {
if let Err(e) = self.check_dyn_import(&module_specifier) {
Expand All @@ -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,
Expand Down
47 changes: 18 additions & 29 deletions core/es_isolate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Isolate>,
loader: Rc<dyn Loader + Unpin>,
loader: Rc<dyn ModuleLoader>,
pub modules: Modules,
pub(crate) next_dyn_import_id: DynImportId,
pub(crate) dyn_import_map:
Expand All @@ -84,7 +73,7 @@ impl DerefMut for EsIsolate {

impl EsIsolate {
pub fn new(
loader: Rc<dyn Loader + Unpin>,
loader: Rc<dyn ModuleLoader>,
startup_data: StartupData,
will_snapshot: bool,
) -> Box<Self> {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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};
Expand All @@ -566,7 +555,7 @@ pub mod tests {
pub count: Arc<AtomicUsize>,
}

impl Loader for ModsLoader {
impl ModuleLoader for ModsLoader {
fn resolve(
&self,
specifier: &str,
Expand All @@ -585,7 +574,7 @@ pub mod tests {
_module_specifier: &ModuleSpecifier,
_maybe_referrer: Option<ModuleSpecifier>,
_is_dyn_import: bool,
) -> Pin<Box<SourceCodeInfoFuture>> {
) -> Pin<Box<ModuleSourceFuture>> {
unreachable!()
}
}
Expand Down Expand Up @@ -665,7 +654,7 @@ pub mod tests {
pub count: Arc<AtomicUsize>,
}

impl Loader for DynImportErrLoader {
impl ModuleLoader for DynImportErrLoader {
fn resolve(
&self,
specifier: &str,
Expand All @@ -684,7 +673,7 @@ pub mod tests {
_module_specifier: &ModuleSpecifier,
_maybe_referrer: Option<ModuleSpecifier>,
_is_dyn_import: bool,
) -> Pin<Box<SourceCodeInfoFuture>> {
) -> Pin<Box<ModuleSourceFuture>> {
async { Err(ErrBox::from(io::Error::from(io::ErrorKind::NotFound))) }
.boxed()
}
Expand Down Expand Up @@ -726,7 +715,7 @@ pub mod tests {
pub count: Arc<AtomicUsize>,
}
impl Loader for DynImportErr2Loader {
impl ModuleLoader for DynImportErr2Loader {
fn resolve(
&self,
specifier: &str,
Expand All @@ -750,8 +739,8 @@ pub mod tests {
&self,
specifier: &ModuleSpecifier,
_maybe_referrer: Option<ModuleSpecifier>,
) -> Pin<Box<SourceCodeInfoFuture>> {
let info = SourceCodeInfo {
) -> Pin<Box<ModuleSourceFuture>> {
let info = ModuleSource {
module_url_specified: specifier.to_string(),
module_url_found: specifier.to_string(),
code: "# not valid JS".to_owned(),
Expand Down Expand Up @@ -811,7 +800,7 @@ pub mod tests {
pub load_count: Arc<AtomicUsize>,
}

impl Loader for DynImportOkLoader {
impl ModuleLoader for DynImportOkLoader {
fn resolve(
&self,
specifier: &str,
Expand All @@ -834,9 +823,9 @@ pub mod tests {
specifier: &ModuleSpecifier,
_maybe_referrer: Option<ModuleSpecifier>,
_is_dyn_import: bool,
) -> Pin<Box<SourceCodeInfoFuture>> {
) -> Pin<Box<ModuleSourceFuture>> {
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(),
Expand Down
54 changes: 37 additions & 17 deletions core/modules.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -20,10 +19,31 @@ use std::rc::Rc;
use std::task::Context;
use std::task::Poll;

pub type SourceCodeInfoFuture =
dyn Future<Output = Result<SourceCodeInfo, ErrBox>>;
/// EsModule source code that will be loaded into V8.
///
/// Users can implement `Into<ModuleInfo>` 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<Output = Result<ModuleSource, ErrBox>>;

pub trait Loader {
pub trait ModuleLoader {
/// Returns an absolute URL.
/// When implementing an spec-complaint VM, this should be exactly the
/// algorithm described here:
Expand All @@ -47,7 +67,7 @@ pub trait Loader {
module_specifier: &ModuleSpecifier,
maybe_referrer: Option<ModuleSpecifier>,
is_dyn_import: bool,
) -> Pin<Box<SourceCodeInfoFuture>>;
) -> Pin<Box<ModuleSourceFuture>>;
}

#[derive(Debug, Eq, PartialEq)]
Expand All @@ -74,8 +94,8 @@ pub struct RecursiveModuleLoad {
// Kind::Main
pub dyn_import_id: Option<DynImportId>,
pub state: LoadState,
pub loader: Rc<dyn Loader + Unpin>,
pub pending: FuturesUnordered<Pin<Box<SourceCodeInfoFuture>>>,
pub loader: Rc<dyn ModuleLoader>,
pub pending: FuturesUnordered<Pin<Box<ModuleSourceFuture>>>,
pub is_pending: HashSet<ModuleSpecifier>,
}

Expand All @@ -84,7 +104,7 @@ impl RecursiveModuleLoad {
pub fn main(
specifier: &str,
code: Option<String>,
loader: Rc<dyn Loader + Unpin>,
loader: Rc<dyn ModuleLoader>,
) -> Self {
let kind = Kind::Main;
let state = LoadState::ResolveMain(specifier.to_owned(), code);
Expand All @@ -95,7 +115,7 @@ impl RecursiveModuleLoad {
id: DynImportId,
specifier: &str,
referrer: &str,
loader: Rc<dyn Loader + Unpin>,
loader: Rc<dyn ModuleLoader>,
) -> Self {
let kind = Kind::DynamicImport;
let state =
Expand All @@ -110,7 +130,7 @@ impl RecursiveModuleLoad {
fn new(
kind: Kind,
state: LoadState,
loader: Rc<dyn Loader + Unpin>,
loader: Rc<dyn ModuleLoader>,
dyn_import_id: Option<DynImportId>,
) -> Self {
Self {
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -174,7 +194,7 @@ impl RecursiveModuleLoad {
}

impl Stream for RecursiveModuleLoad {
type Item = Result<SourceCodeInfo, ErrBox>;
type Item = Result<ModuleSource, ErrBox>;

fn poll_next(
self: Pin<&mut Self>,
Expand Down Expand Up @@ -544,7 +564,7 @@ mod tests {
}

impl Future for DelayedSourceCodeFuture {
type Output = Result<SourceCodeInfo, ErrBox>;
type Output = Result<ModuleSource, ErrBox>;

fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
let inner = self.get_mut();
Expand All @@ -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(),
Expand All @@ -569,7 +589,7 @@ mod tests {
}
}

impl Loader for MockLoader {
impl ModuleLoader for MockLoader {
fn resolve(
&self,
specifier: &str,
Expand Down Expand Up @@ -602,7 +622,7 @@ mod tests {
module_specifier: &ModuleSpecifier,
_maybe_referrer: Option<ModuleSpecifier>,
_is_dyn_import: bool,
) -> Pin<Box<SourceCodeInfoFuture>> {
) -> Pin<Box<ModuleSourceFuture>> {
let mut loads = self.loads.lock().unwrap();
loads.push(module_specifier.to_string());
let url = module_specifier.to_string();
Expand Down Expand Up @@ -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";
"#;
Expand Down

0 comments on commit cfe4369

Please sign in to comment.