Repository: denoland/deno_core
Pull request #398 (2023-12-27 22:33:50)

refactor: ModuleLoader::load can return string or bytes
This commit changes signature of "ModuleLoader::load" and "ModuleSourceFuture"
to allow returning either string or bytes. When modules are evaluated and we received
bytes, we try to parse them as UTF-8 and error out if they're not valid UTF-8.

This is done in preparation for WASM and bytes imports.

Ref #344 
Ref #357


Patch:
diff --git a/core/examples/ts_module_loader.rs b/core/examples/ts_module_loader.rs
index d8fe49b..d78f435 100644
--- a/core/examples/ts_module_loader.rs
+++ b/core/examples/ts_module_loader.rs
@@ -22,6 +22,7 @@ use deno_core::resolve_path;
 use deno_core::JsRuntime;
 use deno_core::ModuleLoader;
 use deno_core::ModuleSource;
+use deno_core::ModuleSourceCode;
 use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
@@ -120,7 +121,7 @@ impl ModuleLoader for TypescriptModuleLoader {
       };
       Ok(ModuleSource::new(
         module_type,
-        code.into(),
+        ModuleSourceCode::String(code.into()),
         module_specifier,
       ))
     }
diff --git a/core/lib.rs b/core/lib.rs
index e8ffd8b..ebc8592 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -95,6 +95,7 @@ pub use crate::modules::ModuleCode;
 pub use crate::modules::ModuleId;
 pub use crate::modules::ModuleLoader;
 pub use crate::modules::ModuleSource;
+pub use crate::modules::ModuleSourceCode;
 pub use crate::modules::ModuleSourceFuture;
 pub use crate::modules::ModuleType;
 pub use crate::modules::NoopModuleLoader;
diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs
index 09c59f3..916cb45 100644
--- a/core/modules/loaders.rs
+++ b/core/modules/loaders.rs
@@ -10,8 +10,10 @@ use crate::modules::ModuleType;
 use crate::modules::ResolutionKind;
 use crate::resolve_import;
 use crate::Extension;
+use crate::ModuleSourceCode;
 
 use anyhow::anyhow;
+use anyhow::Context;
 use anyhow::Error;
 use futures::future::ready;
 use futures::future::FutureExt;
@@ -155,7 +157,11 @@ impl ModuleLoader for ExtModuleLoader {
     let result = source.load();
     match result {
       Ok(code) => {
-        let res = ModuleSource::new(ModuleType::JavaScript, code, specifier);
+        let res = ModuleSource::new(
+          ModuleType::JavaScript,
+          ModuleSourceCode::String(code),
+          specifier,
+        );
         return futures::future::ok(res).boxed_local();
       }
       Err(err) => return futures::future::err(err).boxed_local(),
@@ -211,7 +217,11 @@ impl ModuleLoader for LazyEsmModuleLoader {
     let result = source.load();
     match result {
       Ok(code) => {
-        let res = ModuleSource::new(ModuleType::JavaScript, code, specifier);
+        let res = ModuleSource::new(
+          ModuleType::JavaScript,
+          ModuleSourceCode::String(code),
+          specifier,
+        );
         return futures::future::ok(res).boxed_local();
       }
       Err(err) => return futures::future::err(err).boxed_local(),
@@ -293,8 +303,14 @@ impl ModuleLoader for FsModuleLoader {
         ModuleType::JavaScript
       };
 
-      let code = std::fs::read_to_string(path)?.into();
-      let module = ModuleSource::new(module_type, code, module_specifier);
+      let code = std::fs::read(path).with_context(|| {
+        format!("Failed to load {}", module_specifier.as_str())
+      })?;
+      let module = ModuleSource::new(
+        module_type,
+        ModuleSourceCode::Bytes(code),
+        module_specifier,
+      );
       Ok(module)
     }
 
@@ -347,7 +363,7 @@ impl ModuleLoader for StaticModuleLoader {
     let res = if let Some(code) = self.map.get(module_specifier) {
       Ok(ModuleSource::new(
         ModuleType::JavaScript,
-        code.try_clone().unwrap(),
+        ModuleSourceCode::String(code.try_clone().unwrap()),
         module_specifier,
       ))
     } else {
diff --git a/core/modules/map.rs b/core/modules/map.rs
index 45f1cf7..c6188ba 100644
--- a/core/modules/map.rs
+++ b/core/modules/map.rs
@@ -271,22 +271,24 @@ impl ModuleMap {
 
     let asserted_module_type = AssertedModuleType::from(module_type);
     let maybe_module_id = self.get_id(&module_url_found, asserted_module_type);
-    let module_id = match maybe_module_id {
-      Some(id) => {
-        debug!(
-          "Already-registered module fetched again: {:?}",
-          module_url_found
-        );
-        id
+
+    if let Some(module_id) = maybe_module_id {
+      debug!(
+        "Already-registered module fetched again: {:?}",
+        module_url_found
+      );
+      return Ok(module_id);
+    }
+
+    let code = ModuleSource::get_string_source(module_url_found.as_str(), code)
+      .map_err(ModuleError::Other)?;
+    let module_id = match module_type {
+      ModuleType::JavaScript => {
+        self.new_es_module(scope, main, module_url_found, code, dynamic)?
+      }
+      ModuleType::Json => {
+        self.new_json_module(scope, module_url_found, code)?
       }
-      None => match module_type {
-        ModuleType::JavaScript => {
-          self.new_es_module(scope, main, module_url_found, code, dynamic)?
-        }
-        ModuleType::Json => {
-          self.new_json_module(scope, module_url_found, code)?
-        }
-      },
     };
     Ok(module_id)
   }
@@ -1380,7 +1382,11 @@ impl ModuleMap {
       loader.load(&specifier, None, false).await
     })?;
 
-    self.lazy_load_es_module_from_code(scope, module_specifier, source.code)
+    self.lazy_load_es_module_from_code(
+      scope,
+      module_specifier,
+      ModuleSource::get_string_source(specifier.as_str(), source.code)?,
+    )
   }
 }
 
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index 6271ade..7b53436 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -1,7 +1,9 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+use crate::error::AnyError;
 use crate::fast_string::FastString;
 use crate::module_specifier::ModuleSpecifier;
 use anyhow::bail;
+use anyhow::Context;
 use anyhow::Error;
 use serde::Deserialize;
 use serde::Serialize;
@@ -33,6 +35,18 @@ pub(crate) use map::ModuleMap;
 
 pub type ModuleId = usize;
 pub(crate) type ModuleLoadId = i32;
+
+/// The actual source code returned from the loader. Most embedders should
+/// try to return bytes and let deno_core interpret if the module should be
+/// converted to a string or not.
+#[derive(Debug)]
+pub enum ModuleSourceCode {
+  String(ModuleCode),
+  Bytes(Vec<u8>),
+}
+
+// TODO(bartlomieju): maybe remove it? It might be confusing between `ModuleSourceCode`
+// and `ModuleCode`.
 pub type ModuleCode = FastString;
 pub type ModuleName = FastString;
 
@@ -183,7 +197,7 @@ impl std::fmt::Display for ModuleType {
 // NOTE: This should _not_ be made #[derive(Clone)] unless we take some precautions to avoid excessive string copying.
 #[derive(Debug)]
 pub struct ModuleSource {
-  pub code: ModuleCode,
+  pub code: ModuleSourceCode,
   pub module_type: ModuleType,
   module_url_specified: ModuleName,
   /// If the module was found somewhere other than the specified address, this will be [`Some`].
@@ -194,7 +208,7 @@ impl ModuleSource {
   /// Create a [`ModuleSource`] without a redirect.
   pub fn new(
     module_type: impl Into<ModuleType>,
-    code: ModuleCode,
+    code: ModuleSourceCode,
     specifier: &ModuleSpecifier,
   ) -> Self {
     let module_url_specified = specifier.as_ref().to_owned().into();
@@ -210,7 +224,7 @@ impl ModuleSource {
   /// specifier, the code behaves the same was as `ModuleSource::new`.
   pub fn new_with_redirect(
     module_type: impl Into<ModuleType>,
-    code: ModuleCode,
+    code: ModuleSourceCode,
     specifier: &ModuleSpecifier,
     specifier_found: &ModuleSpecifier,
   ) -> Self {
@@ -231,7 +245,7 @@ impl ModuleSource {
   #[cfg(test)]
   pub fn for_test(code: &'static str, file: impl AsRef<str>) -> Self {
     Self {
-      code: ModuleCode::from_static(code),
+      code: ModuleSourceCode::String(FastString::from_static(code)),
       module_type: ModuleType::JavaScript,
       module_url_specified: file.as_ref().to_owned().into(),
       module_url_found: None,
@@ -253,12 +267,27 @@ impl ModuleSource {
       Some(found.into())
     };
     Self {
-      code: ModuleCode::from_static(code),
+      code: ModuleSourceCode::String(FastString::from_static(code)),
       module_type: ModuleType::JavaScript,
       module_url_specified: specified.into(),
       module_url_found: found,
     }
   }
+
+  pub fn get_string_source(
+    specifier: &str,
+    code: ModuleSourceCode,
+  ) -> Result<ModuleCode, AnyError> {
+    match code {
+      ModuleSourceCode::String(code) => Ok(code),
+      ModuleSourceCode::Bytes(bytes) => {
+        let str_ = String::from_utf8(bytes).with_context(|| {
+          format!("Can't convert source code to string for {}", specifier)
+        })?;
+        Ok(ModuleCode::from(str_))
+      }
+    }
+  }
 }
 
 pub type ModuleSourceFuture = dyn Future<Output = Result<ModuleSource, Error>>;


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index 21fba4f..197a91c 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -7,6 +7,7 @@ use crate::modules::loaders::*;
 use crate::modules::AssertedModuleType;
 use crate::modules::ModuleError;
 use crate::modules::ModuleRequest;
+use crate::modules::ModuleSourceCode;
 use crate::resolve_import;
 use crate::resolve_url;
 use crate::runtime::JsRuntime;
@@ -1355,7 +1356,7 @@ async fn no_duplicate_loads() {
           .unwrap()
       };
       let module_source = ModuleSource {
-        code: ModuleCode::from(source_code),
+        code: ModuleSourceCode::String(ModuleCode::from(source_code)),
         module_type: ModuleType::JavaScript,
         module_url_specified: module_specifier.clone().into(),
         module_url_found: found_specifier.map(|s| s.into()),
diff --git a/testing/checkin/runner/ts_module_loader.rs b/testing/checkin/runner/ts_module_loader.rs
index 125c1b9..365d5df 100644
--- a/testing/checkin/runner/ts_module_loader.rs
+++ b/testing/checkin/runner/ts_module_loader.rs
@@ -17,6 +17,7 @@ use deno_core::ExtensionFileSource;
 use deno_core::ExtensionFileSourceCode;
 use deno_core::ModuleLoader;
 use deno_core::ModuleSource;
+use deno_core::ModuleSourceCode;
 use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
@@ -121,7 +122,7 @@ impl ModuleLoader for TypescriptModuleLoader {
       };
       Ok(ModuleSource::new(
         module_type,
-        code.into(),
+        ModuleSourceCode::String(code.into()),
         module_specifier,
       ))
     }