Repository: denoland/deno_core
Pull request #439 (2024-01-12 00:00:55)

feat: add 'requested_module_type' to ModuleLoader::load
This commit adds additional argument to "ModuleLoader::load" method that
is "RequestedModuleType". This argument can be used by the embedder to 
decide if the loaded module should be refused (eg. trying to load a module
with type "json", but the actual MIME type is not "application/json").

In a follow up I might extract `is_dyn_import` and `requested_module_type`
into a helper struct (maybe `LoadMetadata`).

Ref https://github.com/denoland/deno_core/pull/402


Patch:
diff --git a/core/examples/ts_module_loader.rs b/core/examples/ts_module_loader.rs
index d78f435..a23d66f 100644
--- a/core/examples/ts_module_loader.rs
+++ b/core/examples/ts_module_loader.rs
@@ -26,6 +26,7 @@ use deno_core::ModuleSourceCode;
 use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
+use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::RuntimeOptions;
 use deno_core::SourceMapGetter;
@@ -67,6 +68,7 @@ impl ModuleLoader for TypescriptModuleLoader {
     module_specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let source_maps = self.source_maps.clone();
     fn load(
diff --git a/core/lib.rs b/core/lib.rs
index 924d119..f51d4f7 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -101,6 +101,7 @@ pub use crate::modules::ModuleSourceCode;
 pub use crate::modules::ModuleSourceFuture;
 pub use crate::modules::ModuleType;
 pub use crate::modules::NoopModuleLoader;
+pub use crate::modules::RequestedModuleType;
 pub use crate::modules::ResolutionKind;
 pub use crate::modules::StaticModuleLoader;
 pub use crate::modules::ValidateImportAttributesCb;
diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs
index a74385e..ae7af78 100644
--- a/core/modules/loaders.rs
+++ b/core/modules/loaders.rs
@@ -24,6 +24,8 @@ use std::future::Future;
 use std::pin::Pin;
 use std::rc::Rc;
 
+use super::RequestedModuleType;
+
 pub trait ModuleLoader {
   /// Returns an absolute URL.
   /// When implementing an spec-complaint VM, this should be exactly the
@@ -51,6 +53,7 @@ pub trait ModuleLoader {
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<&ModuleSpecifier>,
     is_dyn_import: bool,
+    requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>>;
 
   /// This hook can be used by implementors to do some preparation
@@ -90,6 +93,7 @@ impl ModuleLoader for NoopModuleLoader {
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let maybe_referrer = maybe_referrer
       .map(|s| s.as_str())
@@ -144,6 +148,7 @@ impl ModuleLoader for ExtModuleLoader {
     specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let sources = self.sources.borrow();
     let source = match sources.get(specifier.as_str()) {
@@ -208,6 +213,7 @@ impl ModuleLoader for LazyEsmModuleLoader {
     specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let sources = self.sources.borrow();
     let source = match sources.get(specifier.as_str()) {
@@ -283,6 +289,7 @@ impl ModuleLoader for FsModuleLoader {
     module_specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dynamic: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     fn load(
       module_specifier: &ModuleSpecifier,
@@ -359,6 +366,7 @@ impl ModuleLoader for StaticModuleLoader {
     module_specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let res = if let Some(code) = self.map.get(module_specifier) {
       Ok(ModuleSource::new(
@@ -439,12 +447,16 @@ impl<L: ModuleLoader> ModuleLoader for TestingModuleLoader<L> {
     module_specifier: &ModuleSpecifier,
     maybe_referrer: Option<&ModuleSpecifier>,
     is_dyn_import: bool,
+    requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     self.load_count.set(self.load_count.get() + 1);
     self.log.borrow_mut().push(module_specifier.clone());
-    self
-      .loader
-      .load(module_specifier, maybe_referrer, is_dyn_import)
+    self.loader.load(
+      module_specifier,
+      maybe_referrer,
+      is_dyn_import,
+      requested_module_type,
+    )
   }
 }
 
diff --git a/core/modules/map.rs b/core/modules/map.rs
index dc15efc..c8b2879 100644
--- a/core/modules/map.rs
+++ b/core/modules/map.rs
@@ -279,6 +279,12 @@ impl ModuleMap {
       module_url_specified
     };
 
+    // TODO(bartlomieju): I have a hunch that this is wrong - write a test
+    // that tries to "confuse" the type system, by first requesting a module
+    // with type `RequestedModuleType::Other("foo".into)``, and then the loader
+    // actually returns `ModuleType::Other("bar".into())`. See if it leads to
+    // unexpected result in how `ModuleMap` is structured and verify how
+    // querying the module map works (`ModuleMap::get_by_id`, `ModuleMap::get_by_name`).
     let requested_module_type = RequestedModuleType::from(module_type.clone());
     let maybe_module_id = self.get_id(&module_url_found, requested_module_type);
 
@@ -1459,7 +1465,9 @@ impl ModuleMap {
 
     let specifier = ModuleSpecifier::parse(module_specifier)?;
     let source = futures::executor::block_on(async {
-      loader.load(&specifier, None, false).await
+      loader
+        .load(&specifier, None, false, RequestedModuleType::None)
+        .await
     })?;
 
     self.lazy_load_es_module_from_code(
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index 4af83a5..3390962 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -340,17 +340,41 @@ pub enum ResolutionKind {
 
 #[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)]
 #[repr(u8)]
-pub(crate) enum RequestedModuleType {
+pub enum RequestedModuleType {
   /// There was no attribute specified in the import statement.
+  ///
+  /// Example:
+  /// ```ignore
+  /// import foo from "./foo.js";
+  ///
+  /// const bar = await import("bar");
+  /// ```
   None,
 
-  /// `type` attribute had value `json`. This is the only known module type
-  /// in `deno_core`. Embedders should use `Other` variant for custom module
+  /// The `type` attribute had value `json`. This is the only known module type
+  /// in `deno_core`.
+  ///
+  /// Embedders should use `Other` variant for custom module
   /// types like `wasm`, `bytes` or `text`.
+  ///
+  /// Example:
+  /// ```ignore
+  /// import jsonData from "./data.json" with { type: "json" };
+  ///
+  /// const jsonData2 = await import"./data2.json", { with { type: "json" } });
+  /// ```
   Json,
 
-  // IMPORTANT: If you add any additional enum values here, you must update `to_v8`` below!
-  /// Non-well-known module type.
+  /// An arbitrary module type. It is up to the embedder to handle (or deny) it.
+  /// If [`CustomModuleEvaluationCb`] was not passed when creating a runtime,
+  /// then all "other" module types cause an error to be returned.
+  ///
+  /// Example:
+  /// ```ignore
+  /// import text from "./log.txt" with { type: "text" };
+  ///
+  /// const imgData = await import(`./images/${name}.png`, { with: { type: "bytes" }});
+  /// ```
   Other(Cow<'static, str>),
 }
 
diff --git a/core/modules/recursive_load.rs b/core/modules/recursive_load.rs
index 059e2e6..65cf8a7 100644
--- a/core/modules/recursive_load.rs
+++ b/core/modules/recursive_load.rs
@@ -256,6 +256,7 @@ impl RecursiveModuleLoad {
             let referrer = referrer.clone();
             let loader = self.loader.clone();
             let is_dynamic_import = self.is_dynamic_import();
+            let requested_module_type = request.requested_module_type.clone();
             let fut = async move {
               // `visited_as_alias` unlike `visited` is checked as late as
               // possible because it can only be populated after completed
@@ -265,7 +266,12 @@ impl RecursiveModuleLoad {
                 return Ok(None);
               }
               let load_result = loader
-                .load(&specifier, Some(&referrer), is_dynamic_import)
+                .load(
+                  &specifier,
+                  Some(&referrer),
+                  is_dynamic_import,
+                  requested_module_type,
+                )
                 .await;
               if let Ok(source) = &load_result {
                 if let Some(found_specifier) = &source.module_url_found {
@@ -307,7 +313,7 @@ impl Stream for RecursiveModuleLoad {
         };
         let module_request = ModuleRequest {
           specifier: module_specifier.to_string(),
-          requested_module_type,
+          requested_module_type: requested_module_type.clone(),
         };
         let load_fut = if let Some(module_id) = inner.root_module_id {
           // If the inner future is already in the map, we might be done (assuming there are no pending
@@ -329,12 +335,14 @@ impl Stream for RecursiveModuleLoad {
           };
           let loader = inner.loader.clone();
           let is_dynamic_import = inner.is_dynamic_import();
+          let requested_module_type = requested_module_type.clone();
           async move {
             let result = loader
               .load(
                 &module_specifier,
                 maybe_referrer.as_ref(),
                 is_dynamic_import,
+                requested_module_type,
               )
               .await;
             result.map(|s| Some((module_request, s)))


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index 928dba3..6d65b5f 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -239,6 +239,7 @@ impl ModuleLoader for MockLoader {
     module_specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let mut loads = self.loads.lock();
     loads.push(module_specifier.to_string());
@@ -1438,6 +1439,7 @@ async fn no_duplicate_loads() {
       module_specifier: &ModuleSpecifier,
       _maybe_referrer: Option<&ModuleSpecifier>,
       _is_dyn_import: bool,
+      _requested_module_type: RequestedModuleType,
     ) -> Pin<Box<ModuleSourceFuture>> {
       let found_specifier =
         if module_specifier.as_str() == "https://example.com/foo.js" {
diff --git a/testing/checkin/runner/ts_module_loader.rs b/testing/checkin/runner/ts_module_loader.rs
index 365d5df..f72a2d9 100644
--- a/testing/checkin/runner/ts_module_loader.rs
+++ b/testing/checkin/runner/ts_module_loader.rs
@@ -21,6 +21,7 @@ use deno_core::ModuleSourceCode;
 use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
+use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::SourceMapGetter;
 
@@ -63,6 +64,7 @@ impl ModuleLoader for TypescriptModuleLoader {
     module_specifier: &ModuleSpecifier,
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
+    _requested_module_type: RequestedModuleType,
   ) -> Pin<Box<ModuleSourceFuture>> {
     let source_maps = self.source_maps.clone();
     fn load(