refactor: Modules and Loader trait (#3791)

* move is_dyn_import argument from Loader::resolve to Loader::load - it was always kind of strange that resolve() checks permissions.
* change argument type from &str to &ModuleSpecifier where applicable

1 files changed, 73 insertions, 61 deletions

diff --git a/core/modules.rs b/core/modules.rs
index 60158d6f1..21c2481dd 100644
--- a/core/modules.rs
+++ b/core/modules.rs
@@ -28,19 +28,25 @@ pub trait Loader: Send + Sync {
   /// When implementing an spec-complaint VM, this should be exactly the
   /// algorithm described here:
   /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
+  ///
+  /// `is_main` can be used to resolve from current working directory or
+  /// apply import map for child imports.
   fn resolve(
     &self,
     specifier: &str,
     referrer: &str,
     is_main: bool,
-    is_dyn_import: bool,
   ) -> Result<ModuleSpecifier, ErrBox>;
 
   /// Given ModuleSpecifier, load its source code.
+  ///
+  /// `is_dyn_import` can be used to check permissions or deny
+  /// dynamic imports altogether.
   fn load(
     &self,
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<ModuleSpecifier>,
+    is_dyn_import: bool,
   ) -> Pin<Box<SourceCodeInfoFuture>>;
 }
 
@@ -121,10 +127,10 @@ impl RecursiveModuleLoad {
   fn add_root(&mut self) -> Result<(), ErrBox> {
     let module_specifier = match self.state {
       LoadState::ResolveMain(ref specifier, _) => {
-        self.loader.resolve(specifier, ".", true, false)?
+        self.loader.resolve(specifier, ".", true)?
       }
       LoadState::ResolveImport(ref specifier, ref referrer) => {
-        self.loader.resolve(specifier, referrer, false, true)?
+        self.loader.resolve(specifier, referrer, false)?
       }
 
       _ => unreachable!(),
@@ -139,7 +145,10 @@ impl RecursiveModuleLoad {
         })
         .boxed()
       }
-      _ => self.loader.load(&module_specifier, None).boxed(),
+      _ => self
+        .loader
+        .load(&module_specifier, None, self.is_dynamic_import())
+        .boxed(),
     };
 
     self.pending.push(load_fut);
@@ -154,7 +163,10 @@ impl RecursiveModuleLoad {
     referrer: ModuleSpecifier,
   ) {
     if !self.is_pending.contains(&specifier) {
-      let fut = self.loader.load(&specifier, Some(referrer));
+      let fut =
+        self
+          .loader
+          .load(&specifier, Some(referrer), self.is_dynamic_import());
       self.pending.push(fut.boxed());
       self.is_pending.insert(specifier);
     }
@@ -192,7 +204,7 @@ pub struct ModuleInfo {
   pub main: bool,
   pub name: String,
   pub handle: v8::Global<v8::Module>,
-  pub import_specifiers: Vec<String>,
+  pub import_specifiers: Vec<ModuleSpecifier>,
 }
 
 /// A symbolic module entity.
@@ -264,7 +276,6 @@ impl ModuleNameMap {
 pub struct Modules {
   pub(crate) info: HashMap<ModuleId, ModuleInfo>,
   by_name: ModuleNameMap,
-  pub(crate) specifier_cache: HashMap<(String, String), ModuleSpecifier>,
 }
 
 impl Modules {
@@ -272,7 +283,6 @@ impl Modules {
     Self {
       info: HashMap::new(),
       by_name: ModuleNameMap::new(),
-      specifier_cache: HashMap::new(),
     }
   }
 
@@ -280,20 +290,16 @@ impl Modules {
     self.by_name.get(name)
   }
 
-  pub fn get_children(&self, id: ModuleId) -> Option<&Vec<String>> {
+  pub fn get_children(&self, id: ModuleId) -> Option<&Vec<ModuleSpecifier>> {
     self.info.get(&id).map(|i| &i.import_specifiers)
   }
 
-  pub fn get_children2(&self, name: &str) -> Option<&Vec<String>> {
-    self.get_id(name).and_then(|id| self.get_children(id))
-  }
-
   pub fn get_name(&self, id: ModuleId) -> Option<&String> {
     self.info.get(&id).map(|i| &i.name)
   }
 
-  pub fn is_registered(&self, name: &str) -> bool {
-    self.by_name.get(name).is_some()
+  pub fn is_registered(&self, specifier: &ModuleSpecifier) -> bool {
+    self.by_name.get(&specifier.to_string()).is_some()
   }
 
   pub fn register(
@@ -302,7 +308,7 @@ impl Modules {
     name: &str,
     main: bool,
     handle: v8::Global<v8::Module>,
-    import_specifiers: Vec<String>,
+    import_specifiers: Vec<ModuleSpecifier>,
   ) {
     let name = String::from(name);
     debug!("register_complete {}", name);
@@ -334,30 +340,8 @@ impl Modules {
     self.info.get(&id)
   }
 
-  pub fn cache_specifier(
-    &mut self,
-    specifier: &str,
-    referrer: &str,
-    resolved_specifier: &ModuleSpecifier,
-  ) {
-    self.specifier_cache.insert(
-      (specifier.to_string(), referrer.to_string()),
-      resolved_specifier.to_owned(),
-    );
-  }
-
-  pub fn get_cached_specifier(
-    &self,
-    specifier: &str,
-    referrer: &str,
-  ) -> Option<&ModuleSpecifier> {
-    self
-      .specifier_cache
-      .get(&(specifier.to_string(), referrer.to_string()))
-  }
-
-  pub fn deps(&self, url: &str) -> Option<Deps> {
-    Deps::new(self, url)
+  pub fn deps(&self, module_specifier: &ModuleSpecifier) -> Option<Deps> {
+    Deps::new(self, module_specifier)
   }
 }
 
@@ -373,9 +357,12 @@ pub struct Deps {
 }
 
 impl Deps {
-  fn new(modules: &Modules, module_name: &str) -> Option<Deps> {
+  fn new(
+    modules: &Modules,
+    module_specifier: &ModuleSpecifier,
+  ) -> Option<Deps> {
     let mut seen = HashSet::new();
-    Self::helper(&mut seen, "".to_string(), true, modules, module_name)
+    Self::helper(&mut seen, "".to_string(), true, modules, module_specifier)
   }
 
   fn helper(
@@ -383,35 +370,37 @@ impl Deps {
     prefix: String,
     is_last: bool,
     modules: &Modules,
-    name: &str, // TODO(ry) rename url
+    module_specifier: &ModuleSpecifier,
   ) -> Option<Deps> {
-    if seen.contains(name) {
+    let name = module_specifier.to_string();
+    if seen.contains(&name) {
       Some(Deps {
-        name: name.to_string(),
+        name,
         prefix,
         deps: None,
         is_last,
       })
     } else {
-      let children = modules.get_children2(name)?;
+      let mod_id = modules.get_id(&name)?;
+      let children = modules.get_children(mod_id).unwrap();
       seen.insert(name.to_string());
       let child_count = children.len();
       let deps: Vec<Deps> = children
         .iter()
         .enumerate()
-        .map(|(index, dep_name)| {
+        .map(|(index, dep_specifier)| {
           let new_is_last = index == child_count - 1;
           let mut new_prefix = prefix.clone();
           new_prefix.push(if is_last { ' ' } else { '│' });
           new_prefix.push(' ');
 
-          Self::helper(seen, new_prefix, new_is_last, modules, dep_name)
+          Self::helper(seen, new_prefix, new_is_last, modules, dep_specifier)
         })
         // If any of the children are missing, return None.
         .collect::<Option<_>>()?;
 
       Some(Deps {
-        name: name.to_string(),
+        name,
         prefix,
         deps: Some(deps),
         is_last,
@@ -565,7 +554,6 @@ mod tests {
       specifier: &str,
       referrer: &str,
       _is_root: bool,
-      _is_dyn_import: bool,
     ) -> Result<ModuleSpecifier, ErrBox> {
       let referrer = if referrer == "." {
         "file:///"
@@ -592,6 +580,7 @@ mod tests {
       &self,
       module_specifier: &ModuleSpecifier,
       _maybe_referrer: Option<ModuleSpecifier>,
+      _is_dyn_import: bool,
     ) -> Pin<Box<SourceCodeInfoFuture>> {
       let mut loads = self.loads.lock().unwrap();
       loads.push(module_specifier.to_string());
@@ -666,10 +655,19 @@ mod tests {
     let d_id = modules.get_id("file:///d.js").unwrap();
     assert_eq!(
       modules.get_children(a_id),
-      Some(&vec!["/b.js".to_string(), "/c.js".to_string()])
+      Some(&vec![
+        ModuleSpecifier::resolve_url("file:///b.js").unwrap(),
+        ModuleSpecifier::resolve_url("file:///c.js").unwrap()
+      ])
+    );
+    assert_eq!(
+      modules.get_children(b_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///c.js").unwrap()])
+    );
+    assert_eq!(
+      modules.get_children(c_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///d.js").unwrap()])
     );
-    assert_eq!(modules.get_children(b_id), Some(&vec!["/c.js".to_string()]));
-    assert_eq!(modules.get_children(c_id), Some(&vec!["/d.js".to_string()]));
     assert_eq!(modules.get_children(d_id), Some(&vec![]));
   }
 
@@ -719,12 +717,16 @@ mod tests {
 
       assert_eq!(
         modules.get_children(circular1_id),
-        Some(&vec!["/circular2.js".to_string()])
+        Some(&vec![
+          ModuleSpecifier::resolve_url("file:///circular2.js").unwrap()
+        ])
       );
 
       assert_eq!(
         modules.get_children(circular2_id),
-        Some(&vec!["/circular3.js".to_string()])
+        Some(&vec![
+          ModuleSpecifier::resolve_url("file:///circular3.js").unwrap()
+        ])
       );
 
       assert!(modules.get_id("file:///circular3.js").is_some());
@@ -732,8 +734,8 @@ mod tests {
       assert_eq!(
         modules.get_children(circular3_id),
         Some(&vec![
-          "/circular1.js".to_string(),
-          "/circular2.js".to_string()
+          ModuleSpecifier::resolve_url("file:///circular1.js").unwrap(),
+          ModuleSpecifier::resolve_url("file:///circular2.js").unwrap()
         ])
       );
     }
@@ -923,17 +925,27 @@ mod tests {
 
     assert_eq!(
       modules.get_children(main_id),
-      Some(&vec!["/b.js".to_string(), "/c.js".to_string()])
+      Some(&vec![
+        ModuleSpecifier::resolve_url("file:///b.js").unwrap(),
+        ModuleSpecifier::resolve_url("file:///c.js").unwrap()
+      ])
+    );
+    assert_eq!(
+      modules.get_children(b_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///c.js").unwrap()])
+    );
+    assert_eq!(
+      modules.get_children(c_id),
+      Some(&vec![ModuleSpecifier::resolve_url("file:///d.js").unwrap()])
     );
-    assert_eq!(modules.get_children(b_id), Some(&vec!["/c.js".to_string()]));
-    assert_eq!(modules.get_children(c_id), Some(&vec!["/d.js".to_string()]));
     assert_eq!(modules.get_children(d_id), Some(&vec![]));
   }
 
   #[test]
   fn empty_deps() {
     let modules = Modules::new();
-    assert!(modules.deps("foo").is_none());
+    let specifier = ModuleSpecifier::resolve_url("file:///foo").unwrap();
+    assert!(modules.deps(&specifier).is_none());
   }
 
   /* TODO(bartlomieju): reenable