Refactor module resolving (#2493)

Adds ModuleSpecifier, which wraps a URL. This is now passed around instead of
specifier and resolver strings.

cli/compiler.rs

@@ -247,8 +247,8 @@ mod tests {
   fn test_compile_sync() {
     tokio_util::init(|| {
       let specifier = "./tests/002_hello.ts";
-      use crate::worker;
-      let module_name = worker::root_specifier_to_url(specifier)
+      use crate::module_specifier::ModuleSpecifier;
+      let module_name = ModuleSpecifier::resolve_root(specifier)
         .unwrap()
         .to_string();
 
@@ -294,8 +294,8 @@ mod tests {
   #[test]
   fn test_bundle_async() {
     let specifier = "./tests/002_hello.ts";
-    use crate::worker;
-    let module_name = worker::root_specifier_to_url(specifier)
+    use crate::module_specifier::ModuleSpecifier;
+    let module_name = ModuleSpecifier::resolve_root(specifier)
       .unwrap()
       .to_string();
 

cli/flags.rs

@@ -502,8 +502,8 @@ pub enum DenoSubcommand {
 }
 
 fn get_default_bundle_filename(source_file: &str) -> String {
-  use crate::worker::root_specifier_to_url;
-  let url = root_specifier_to_url(source_file).unwrap();
+  use crate::module_specifier::ModuleSpecifier;
+  let url = ModuleSpecifier::resolve_root(source_file).unwrap().to_url();
   let path_segments = url.path_segments().unwrap();
   let last = path_segments.last().unwrap();
   String::from(last.trim_end_matches(".ts").trim_end_matches(".js"))

cli/main.rs

@@ -27,6 +27,7 @@ mod http_body;
 mod http_util;
 mod import_map;
 pub mod js_errors;
+mod module_specifier;
 pub mod msg;
 pub mod msg_util;
 pub mod ops;
@@ -45,9 +46,9 @@ pub mod worker;
 
 use crate::compiler::bundle_async;
 use crate::errors::RustOrJsError;
+use crate::module_specifier::ModuleSpecifier;
 use crate::progress::Progress;
 use crate::state::ThreadSafeState;
-use crate::worker::root_specifier_to_url;
 use crate::worker::Worker;
 use deno::v8_set_flags;
 use flags::DenoFlags;
@@ -98,10 +99,12 @@ where
 
 pub fn print_file_info(
   worker: Worker,
-  url: &str,
+  module_specifier: &ModuleSpecifier,
 ) -> impl Future<Item = Worker, Error = ()> {
-  state::fetch_module_meta_data_and_maybe_compile_async(&worker.state, url, ".")
-    .and_then(move |out| {
+  state::fetch_module_meta_data_and_maybe_compile_async(
+    &worker.state,
+    module_specifier,
+  ).and_then(move |out| {
     println!("{} {}", ansi::bold("local:".to_string()), &(out.filename));
 
     println!(
@@ -193,10 +196,8 @@ fn fetch_or_info_command(
     js_check(worker.execute("denoMain()"));
     debug!("main_module {}", main_module);
 
-    let main_url = root_specifier_to_url(&main_module).unwrap();
-
     worker
-      .execute_mod_async(&main_url, true)
+      .execute_mod_async(&main_module, true)
       .map_err(print_err_and_exit)
       .and_then(move |()| {
         if print_info {
@@ -269,11 +270,10 @@ fn bundle_command(flags: DenoFlags, argv: Vec<String>) {
   let (mut _worker, state) = create_worker_and_state(flags, argv);
 
   let main_module = state.main_module().unwrap();
-  let main_url = root_specifier_to_url(&main_module).unwrap();
   assert!(state.argv.len() >= 3);
   let out_file = state.argv[2].clone();
   debug!(">>>>> bundle_async START");
-  let bundle_future = bundle_async(state, main_url.to_string(), out_file)
+  let bundle_future = bundle_async(state, main_module.to_string(), out_file)
     .map_err(|e| {
       debug!("diagnostics returned, exiting!");
       eprintln!("\n{}", e.to_string());
@@ -313,10 +313,8 @@ fn run_script(flags: DenoFlags, argv: Vec<String>) {
     js_check(worker.execute("denoMain()"));
     debug!("main_module {}", main_module);
 
-    let main_url = root_specifier_to_url(&main_module).unwrap();
-
     worker
-      .execute_mod_async(&main_url, false)
+      .execute_mod_async(&main_module, false)
       .and_then(move |()| {
         worker.then(|result| {
           js_check(result);

cli/module_specifier.rs

+use std::fmt;
+use url::Url;
+
+#[derive(Debug, Clone, PartialEq)]
+/// Resolved module specifier
+pub struct ModuleSpecifier(Url);
+
+impl ModuleSpecifier {
+  pub fn to_url(&self) -> Url {
+    self.0.clone()
+  }
+  /// Resolves module using this algorithm:
+  /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
+  pub fn resolve(
+    specifier: &str,
+    base: &str,
+  ) -> Result<ModuleSpecifier, url::ParseError> {
+    // 1. Apply the URL parser to specifier. If the result is not failure, return
+    //    the result.
+    // let specifier = parse_local_or_remote(specifier)?.to_string();
+    if let Ok(url) = Url::parse(specifier) {
+      return Ok(ModuleSpecifier(url));
+    }
+
+    // 2. If specifier does not start with the character U+002F SOLIDUS (/), the
+    //    two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), or the
+    //    three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F
+    //    SOLIDUS (../), return failure.
+    if !specifier.starts_with('/')
+      && !specifier.starts_with("./")
+      && !specifier.starts_with("../")
+    {
+      // TODO This is (probably) not the correct error to return here.
+      // TODO: This error is very not-user-friendly
+      return Err(url::ParseError::RelativeUrlWithCannotBeABaseBase);
+    }
+
+    // 3. Return the result of applying the URL parser to specifier with base URL
+    //    as the base URL.
+    let base_url = Url::parse(base)?;
+    let u = base_url.join(&specifier)?;
+    Ok(ModuleSpecifier(u))
+  }
+
+  /// Takes a string representing a path or URL to a module, but of the type
+  /// passed through the command-line interface for the main module. This is
+  /// slightly different than specifiers used in import statements: "foo.js" for
+  /// example is allowed here, whereas in import statements a leading "./" is
+  /// required ("./foo.js"). This function is aware of the current working
+  /// directory and returns an absolute URL.
+  pub fn resolve_root(
+    root_specifier: &str,
+  ) -> Result<ModuleSpecifier, url::ParseError> {
+    if let Ok(url) = Url::parse(root_specifier) {
+      Ok(ModuleSpecifier(url))
+    } else {
+      let cwd = std::env::current_dir().unwrap();
+      let base = Url::from_directory_path(cwd).unwrap();
+      let url = base.join(root_specifier)?;
+      Ok(ModuleSpecifier(url))
+    }
+  }
+}
+
+impl From<Url> for ModuleSpecifier {
+  fn from(url: Url) -> Self {
+    ModuleSpecifier(url)
+  }
+}
+
+impl fmt::Display for ModuleSpecifier {
+  fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    self.0.fmt(f)
+  }
+}
+
+impl PartialEq<String> for ModuleSpecifier {
+  fn eq(&self, other: &String) -> bool {
+    &self.to_string() == other
+  }
+}

cli/ops.rs

@@ -10,6 +10,7 @@ use crate::fs as deno_fs;
 use crate::http_util;
 use crate::js_errors::apply_source_map;
 use crate::js_errors::JSErrorColor;
+use crate::module_specifier::ModuleSpecifier;
 use crate::msg;
 use crate::msg_util;
 use crate::rand;
@@ -24,12 +25,10 @@ use crate::state::ThreadSafeState;
 use crate::tokio_util;
 use crate::tokio_write;
 use crate::version;
-use crate::worker::root_specifier_to_url;
 use crate::worker::Worker;
 use deno::js_check;
 use deno::Buf;
 use deno::JSError;
-//use deno::Loader;
 use deno::Op;
 use deno::PinnedBuf;
 use flatbuffers::FlatBufferBuilder;
@@ -341,7 +340,9 @@ fn op_start(
   let deno_version = version::DENO;
   let deno_version_off = builder.create_string(deno_version);
 
-  let main_module = state.main_module().map(|m| builder.create_string(&m));
+  let main_module = state
+    .main_module()
+    .map(|m| builder.create_string(&m.to_string()));
 
   let xeval_delim = state
     .flags
@@ -507,7 +508,7 @@ fn op_fetch_module_meta_data(
     Some(import_map) => {
       match import_map.resolve(specifier, referrer) {
         Ok(result) => match result {
-          Some(url) => url.clone(),
+          Some(module_specifier) => module_specifier.to_string(),
           None => specifier.to_string(),
         },
         Err(err) => panic!("error resolving using import map: {:?}", err), // TODO: this should be coerced to DenoError
@@ -2082,11 +2083,11 @@ fn op_create_worker(
   js_check(worker.execute("denoMain()"));
   js_check(worker.execute("workerMain()"));
 
-  let op = root_specifier_to_url(specifier)
-    .and_then(|specifier_url| {
+  let op = ModuleSpecifier::resolve_root(specifier)
+    .and_then(|module_specifier| {
       Ok(
         worker
-          .execute_mod_async(&specifier_url, false)
+          .execute_mod_async(&module_specifier, false)
           .and_then(move |()| {
             let mut workers_tl = parent_state.workers.lock().unwrap();
             workers_tl.insert(rid, worker.shared());

cli/state.rs

@@ -7,13 +7,13 @@ use crate::errors::DenoResult;
 use crate::flags;
 use crate::global_timer::GlobalTimer;
 use crate::import_map::ImportMap;
+use crate::module_specifier::ModuleSpecifier;
 use crate::msg;
 use crate::ops;
 use crate::permissions::DenoPermissions;
 use crate::progress::Progress;
 use crate::resources;
 use crate::resources::ResourceId;
-use crate::worker::resolve_module_spec;
 use crate::worker::Worker;
 use deno::Buf;
 use deno::Loader;
@@ -60,7 +60,7 @@ pub struct ThreadSafeState(Arc<State>);
 #[cfg_attr(feature = "cargo-clippy", allow(stutter))]
 pub struct State {
   pub modules: Arc<Mutex<deno::Modules>>,
-  pub main_module: Option<String>,
+  pub main_module: Option<ModuleSpecifier>,
   pub dir: deno_dir::DenoDir,
   pub argv: Vec<String>,
   pub permissions: DenoPermissions,
@@ -113,24 +113,21 @@ impl ThreadSafeState {
 
 pub fn fetch_module_meta_data_and_maybe_compile_async(
   state: &ThreadSafeState,
-  specifier: &str,
-  referrer: &str,
+  module_specifier: &ModuleSpecifier,
 ) -> impl Future<Item = ModuleMetaData, Error = DenoError> {
   let state_ = state.clone();
-  let specifier = specifier.to_string();
-  let referrer = referrer.to_string();
-  let is_root = referrer == ".";
-
-  let f =
-    futures::future::result(state.resolve(&specifier, &referrer, is_root));
-  f.and_then(move |module_id| {
-    let use_cache = !state_.flags.reload || state_.has_compiled(&module_id);
+  let use_cache =
+    !state_.flags.reload || state_.has_compiled(&module_specifier.to_string());
   let no_fetch = state_.flags.no_fetch;
 
   state_
     .dir
-      .fetch_module_meta_data_async(&specifier, &referrer, use_cache, no_fetch)
-      .and_then(move |out| {
+    .fetch_module_meta_data_async(
+      &module_specifier.to_string(),
+      ".",
+      use_cache,
+      no_fetch,
+    ).and_then(move |out| {
       if out.media_type == msg::MediaType::TypeScript
         && !out.has_output_code_and_source_map()
       {
@@ -150,7 +147,6 @@ pub fn fetch_module_meta_data_and_maybe_compile_async(
         Either::B(futures::future::ok(out))
       }
     })
-  })
 }
 
 impl Loader for ThreadSafeState {
@@ -164,28 +160,25 @@ impl Loader for ThreadSafeState {
   ) -> Result<String, Self::Error> {
     if !is_root {
       if let Some(import_map) = &self.import_map {
-        match import_map.resolve(specifier, referrer) {
-          Ok(result) => {
+        let result = import_map.resolve(specifier, referrer)?;
         if result.is_some() {
-              return Ok(result.unwrap());
-            }
-          }
-          Err(err) => {
-            // TODO(bartlomieju): this should be coerced to DenoError
-            panic!("error resolving using import map: {:?}", err);
-          }
+          return Ok(result.unwrap().to_string());
         }
       }
     }
 
-    resolve_module_spec(specifier, referrer).map_err(DenoError::from)
+    let module_specifier =
+      ModuleSpecifier::resolve(specifier, referrer).map_err(DenoError::from)?;
+    Ok(module_specifier.to_string())
   }
 
   /// Given an absolute url, load its source code.
   fn load(&self, url: &str) -> Box<deno::SourceCodeInfoFuture<Self::Error>> {
     self.metrics.resolve_count.fetch_add(1, Ordering::SeqCst);
+    let module_specifier = ModuleSpecifier::resolve_root(url)
+      .expect("should already been properly resolved");
     Box::new(
-      fetch_module_meta_data_and_maybe_compile_async(self, url, ".")
+      fetch_module_meta_data_and_maybe_compile_async(self, &module_specifier)
         .map_err(|err| {
           eprintln!("{}", err);
           err
@@ -256,18 +249,15 @@ impl ThreadSafeState {
     let dir =
       deno_dir::DenoDir::new(custom_root, &config, progress.clone()).unwrap();
 
-    let main_module: Option<String> = if argv_rest.len() <= 1 {
+    let main_module: Option<ModuleSpecifier> = if argv_rest.len() <= 1 {
       None
     } else {
-      let specifier = argv_rest[1].clone();
-      let referrer = ".";
-      // TODO: does this really have to be resolved by DenoDir?
-      //  Maybe we can call `resolve_module_spec`
-      match dir.resolve_module_url(&specifier, referrer) {
-        Ok(url) => Some(url.to_string()),
+      let root_specifier = argv_rest[1].clone();
+      match ModuleSpecifier::resolve_root(&root_specifier) {
+        Ok(specifier) => Some(specifier),
         Err(e) => {
-          debug!("Potentially swallowed error {}", e);
-          None
+          // TODO: handle unresolvable specifier
+          panic!("Unable to resolve root specifier: {:?}", e);
         }
       }
     };
@@ -275,11 +265,11 @@ impl ThreadSafeState {
     let mut import_map = None;
     if let Some(file_name) = &flags.import_map_path {
       let base_url = match &main_module {
-        Some(url) => url,
+        Some(module_specifier) => module_specifier.clone(),
         None => unreachable!(),
       };
 
-      match ImportMap::load(base_url, file_name) {
+      match ImportMap::load(&base_url.to_string(), file_name) {
         Ok(map) => import_map = Some(map),
         Err(err) => {
           println!("{:?}", err);
@@ -319,9 +309,9 @@ impl ThreadSafeState {
   }
 
   /// Read main module from argv
-  pub fn main_module(&self) -> Option<String> {
+  pub fn main_module(&self) -> Option<ModuleSpecifier> {
     match &self.main_module {
-      Some(url) => Some(url.to_string()),
+      Some(module_specifier) => Some(module_specifier.clone()),
       None => None,
     }
   }

cli/worker.rs

@@ -2,6 +2,7 @@
 use crate::errors::DenoError;
 use crate::errors::RustOrJsError;
 use crate::js_errors;
+use crate::module_specifier::ModuleSpecifier;
 use crate::state::ThreadSafeState;
 use crate::tokio_util;
 use deno;
@@ -11,7 +12,6 @@ use futures::Async;
 use futures::Future;
 use std::sync::Arc;
 use std::sync::Mutex;
-use url::Url;
 
 /// Wraps deno::Isolate to provide source maps, ops for the CLI, and
 /// high-level module loading
@@ -57,7 +57,7 @@ impl Worker {
   /// Executes the provided JavaScript module.
   pub fn execute_mod_async(
     &mut self,
-    js_url: &Url,
+    module_specifier: &ModuleSpecifier,
     is_prefetch: bool,
   ) -> impl Future<Item = (), Error = RustOrJsError> {
     let worker = self.clone();
@@ -65,8 +65,12 @@ impl Worker {
     let loader = self.state.clone();
     let isolate = self.isolate.clone();
     let modules = self.state.modules.clone();
-    let recursive_load =
-      deno::RecursiveLoad::new(js_url.as_str(), loader, isolate, modules);
+    let recursive_load = deno::RecursiveLoad::new(
+      &module_specifier.to_string(),
+      loader,
+      isolate,
+      modules,
+    );
     recursive_load
       .and_then(move |id| -> Result<(), deno::JSErrorOr<DenoError>> {
         worker.state.progress.done();
@@ -96,10 +100,10 @@ impl Worker {
   /// Executes the provided JavaScript module.
   pub fn execute_mod(
     &mut self,
-    js_url: &Url,
+    module_specifier: &ModuleSpecifier,
     is_prefetch: bool,
   ) -> Result<(), RustOrJsError> {
-    tokio_util::block_on(self.execute_mod_async(js_url, is_prefetch))
+    tokio_util::block_on(self.execute_mod_async(module_specifier, is_prefetch))
   }
 
   /// Applies source map to the error.
@@ -108,58 +112,6 @@ impl Worker {
   }
 }
 
-// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier
-// TODO(ry) Add tests.
-// TODO(ry) Move this to core?
-pub fn resolve_module_spec(
-  specifier: &str,
-  base: &str,
-) -> Result<String, url::ParseError> {
-  // 1. Apply the URL parser to specifier. If the result is not failure, return
-  //    the result.
-  // let specifier = parse_local_or_remote(specifier)?.to_string();
-  if let Ok(specifier_url) = Url::parse(specifier) {
-    return Ok(specifier_url.to_string());
-  }
-
-  // 2. If specifier does not start with the character U+002F SOLIDUS (/), the
-  //    two-character sequence U+002E FULL STOP, U+002F SOLIDUS (./), or the
-  //    three-character sequence U+002E FULL STOP, U+002E FULL STOP, U+002F
-  //    SOLIDUS (../), return failure.
-  if !specifier.starts_with('/')
-    && !specifier.starts_with("./")
-    && !specifier.starts_with("../")
-  {
-    // TODO(ry) This is (probably) not the correct error to return here.
-    return Err(url::ParseError::RelativeUrlWithCannotBeABaseBase);
-  }
-
-  // 3. Return the result of applying the URL parser to specifier with base URL
-  //    as the base URL.
-  let base_url = Url::parse(base)?;
-  let u = base_url.join(&specifier)?;
-  Ok(u.to_string())
-}
-
-/// Takes a string representing a path or URL to a module, but of the type
-/// passed through the command-line interface for the main module. This is
-/// slightly different than specifiers used in import statements: "foo.js" for
-/// example is allowed here, whereas in import statements a leading "./" is
-/// required ("./foo.js"). This function is aware of the current working
-/// directory and returns an absolute URL.
-pub fn root_specifier_to_url(
-  root_specifier: &str,
-) -> Result<Url, url::ParseError> {
-  let maybe_url = Url::parse(root_specifier);
-  if let Ok(url) = maybe_url {
-    Ok(url)
-  } else {
-    let cwd = std::env::current_dir().unwrap();
-    let base = Url::from_directory_path(cwd).unwrap();
-    base.join(root_specifier)
-  }
-}
-
 impl Future for Worker {
   type Item = ();
   type Error = JSError;
@@ -186,12 +138,9 @@ mod tests {
 
   #[test]
   fn execute_mod_esm_imports_a() {
-    let filename = std::env::current_dir()
-      .unwrap()
-      .join("tests/esm_imports_a.js");
-    let js_url = Url::from_file_path(filename).unwrap();
-
-    let argv = vec![String::from("./deno"), js_url.to_string()];
+    let module_specifier =
+      ModuleSpecifier::resolve_root("tests/esm_imports_a.js").unwrap();
+    let argv = vec![String::from("./deno"), module_specifier.to_string()];
     let state = ThreadSafeState::new(
       flags::DenoFlags::default(),
       argv,
@@ -202,7 +151,7 @@ mod tests {
     tokio_util::run(lazy(move || {
       let mut worker =
         Worker::new("TEST".to_string(), StartupData::None, state);
-      let result = worker.execute_mod(&js_url, false);
+      let result = worker.execute_mod(&module_specifier, false);
       if let Err(err) = result {
         eprintln!("execute_mod err {:?}", err);
       }
@@ -217,10 +166,9 @@ mod tests {
 
   #[test]
   fn execute_mod_circular() {
-    let filename = std::env::current_dir().unwrap().join("tests/circular1.js");
-    let js_url = Url::from_file_path(filename).unwrap();
-
-    let argv = vec![String::from("./deno"), js_url.to_string()];
+    let module_specifier =
+      ModuleSpecifier::resolve_root("tests/circular1.js").unwrap();
+    let argv = vec![String::from("./deno"), module_specifier.to_string()];
     let state = ThreadSafeState::new(
       flags::DenoFlags::default(),
       argv,
@@ -231,7 +179,7 @@ mod tests {
     tokio_util::run(lazy(move || {
       let mut worker =
         Worker::new("TEST".to_string(), StartupData::None, state);
-      let result = worker.execute_mod(&js_url, false);
+      let result = worker.execute_mod(&module_specifier, false);
       if let Err(err) = result {
         eprintln!("execute_mod err {:?}", err);
       }
@@ -246,11 +194,9 @@ mod tests {
 
   #[test]
   fn execute_006_url_imports() {
-    let filename = std::env::current_dir()
-      .unwrap()
-      .join("tests/006_url_imports.ts");
-    let js_url = Url::from_file_path(filename).unwrap();
-    let argv = vec![String::from("deno"), js_url.to_string()];
+    let module_specifier =
+      ModuleSpecifier::resolve_root("tests/006_url_imports.ts").unwrap();
+    let argv = vec![String::from("deno"), module_specifier.to_string()];
     let mut flags = flags::DenoFlags::default();
     flags.reload = true;
     let state =
@@ -263,7 +209,7 @@ mod tests {
         state,
       );
       js_check(worker.execute("denoMain()"));
-      let result = worker.execute_mod(&js_url, false);
+      let result = worker.execute_mod(&module_specifier, false);
       if let Err(err) = result {
         eprintln!("execute_mod err {:?}", err);
       }
@@ -378,8 +324,9 @@ mod tests {
     tokio_util::init(|| {
       // "foo" is not a vailid module specifier so this should return an error.
       let mut worker = create_test_worker();
-      let js_url = root_specifier_to_url("does-not-exist").unwrap();
-      let result = worker.execute_mod_async(&js_url, false).wait();
+      let module_specifier =
+        ModuleSpecifier::resolve_root("does-not-exist").unwrap();
+      let result = worker.execute_mod_async(&module_specifier, false).wait();
       assert!(result.is_err());
     })
   }
@@ -390,8 +337,9 @@ mod tests {
       // This assumes cwd is project root (an assumption made throughout the
       // tests).
       let mut worker = create_test_worker();
-      let js_url = root_specifier_to_url("./tests/002_hello.ts").unwrap();
-      let result = worker.execute_mod_async(&js_url, false).wait();
+      let module_specifier =
+        ModuleSpecifier::resolve_root("./tests/002_hello.ts").unwrap();
+      let result = worker.execute_mod_async(&module_specifier, false).wait();
       assert!(result.is_ok());
     })
   }