Repository: denoland/deno_core
Pull request #823 (2024-07-13 00:41:37)

feat: Depcrate `SourceMapGetter`, move methods to `ModuleLoader`
This commit deprecates `SourceMapGetter` trait, `JsRuntime::source_map_getter`
option and updates `ModuleLoader` trait to have the same methods as
`SourceMapGetter`.

In real usage it's always implementor of `ModuleLoader` that also implements
`SourceMapGetter`. It makes little sense to keep these two trait separate.

This will open up doors for native source map support that is being
developed in https://github.com/denoland/deno_core/pull/819.

`SourceMapGetter` is scheduled to be removed in 0.297.0.

codecov-commenter: ## [Codecov](https://app.codecov.io/gh/denoland/deno_core/pull/823?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) Report
Attention: Patch coverage is `57.14286%` with `15 lines` in your changes missing coverage. Please review.
> Project coverage is 81.53%. Comparing base [(`0c7f83e`)](https://app.codecov.io/gh/denoland/deno_core/commit/0c7f83ea8daabe8f541fc93ae97529dc446f5f24?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) to head [(`562768e`)](https://app.codecov.io/gh/denoland/deno_core/commit/562768e4ff824159ed8a775b444fe7c8eca45995?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).
> Report is 60 commits behind head on main.

| [Files](https://app.codecov.io/gh/denoland/deno_core/pull/823?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) | Patch % | Lines |
|---|---|---|
| [core/source\_map.rs](https://app.codecov.io/gh/denoland/deno_core/pull/823?src=pr&el=tree&filepath=core%2Fsource_map.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-Y29yZS9zb3VyY2VfbWFwLnJz) | 50.00% | [8 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/823?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |
| [core/modules/loaders.rs](https://app.codecov.io/gh/denoland/deno_core/pull/823?src=pr&el=tree&filepath=core%2Fmodules%2Floaders.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland#diff-Y29yZS9tb2R1bGVzL2xvYWRlcnMucnM=) | 30.00% | [7 Missing :warning: ](https://app.codecov.io/gh/denoland/deno_core/pull/823?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
+ Coverage   81.43%   81.53%   +0.09%     
==========================================
  Files          97       98       +1     
  Lines       23877    24324     +447     
==========================================
+ Hits        19445    19833     +388     
- Misses       4432     4491      +59     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/denoland/deno_core/pull/823?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=denoland).


Patch:
diff --git a/core/examples/ts_module_loader.rs b/core/examples/ts_module_loader.rs
index 816b06b..6e99507 100644
--- a/core/examples/ts_module_loader.rs
+++ b/core/examples/ts_module_loader.rs
@@ -29,29 +29,16 @@ use deno_core::ModuleType;
 use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::RuntimeOptions;
-use deno_core::SourceMapGetter;
 
-#[derive(Clone)]
-struct SourceMapStore(Rc<RefCell<HashMap<String, Vec<u8>>>>);
-
-impl SourceMapGetter for SourceMapStore {
-  fn get_source_map(&self, specifier: &str) -> Option<Vec<u8>> {
-    self.0.borrow().get(specifier).cloned()
-  }
-
-  fn get_source_line(
-    &self,
-    _file_name: &str,
-    _line_number: usize,
-  ) -> Option<String> {
-    None
-  }
-}
+// TODO(bartlomieju): this is duplicated in `testing/checkin`
+type SourceMapStore = Rc<RefCell<HashMap<String, Vec<u8>>>>;
 
+// TODO(bartlomieju): this is duplicated in `testing/checkin`
 struct TypescriptModuleLoader {
   source_maps: SourceMapStore,
 }
 
+// TODO(bartlomieju): this is duplicated in `testing/checkin`
 impl ModuleLoader for TypescriptModuleLoader {
   fn resolve(
     &self,
@@ -112,7 +99,6 @@ impl ModuleLoader for TypescriptModuleLoader {
         })?;
         let source_map = res.source_map.unwrap();
         source_maps
-          .0
           .borrow_mut()
           .insert(module_specifier.to_string(), source_map.into_bytes());
         res.text
@@ -129,6 +115,10 @@ impl ModuleLoader for TypescriptModuleLoader {
 
     ModuleLoadResponse::Sync(load(source_maps, module_specifier))
   }
+
+  fn get_source_map(&self, specifier: &str) -> Option<Vec<u8>> {
+    self.source_maps.borrow().get(specifier).cloned()
+  }
 }
 
 fn main() -> Result<(), Error> {
@@ -140,13 +130,12 @@ fn main() -> Result<(), Error> {
   let main_url = &args[1];
   println!("Run {main_url}");
 
-  let source_map_store = SourceMapStore(Rc::new(RefCell::new(HashMap::new())));
+  let source_map_store = Rc::new(RefCell::new(HashMap::new()));
 
   let mut js_runtime = JsRuntime::new(RuntimeOptions {
     module_loader: Some(Rc::new(TypescriptModuleLoader {
-      source_maps: source_map_store.clone(),
+      source_maps: source_map_store,
     })),
-    source_map_getter: Some(Rc::new(source_map_store)),
     ..Default::default()
   });
 
diff --git a/core/lib.rs b/core/lib.rs
index 31d6c89..8014af2 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -161,6 +161,7 @@ pub use crate::runtime::MODULE_MAP_SLOT_INDEX;
 pub use crate::runtime::V8_WRAPPER_OBJECT_INDEX;
 pub use crate::runtime::V8_WRAPPER_TYPE_INDEX;
 pub use crate::source_map::SourceMapData;
+#[allow(deprecated)]
 pub use crate::source_map::SourceMapGetter;
 pub use crate::tasks::V8CrossThreadTaskSpawner;
 pub use crate::tasks::V8TaskSpawner;
diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs
index edab4a8..88c8dbb 100644
--- a/core/modules/loaders.rs
+++ b/core/modules/loaders.rs
@@ -95,6 +95,21 @@ pub trait ModuleLoader {
   ) -> Pin<Box<dyn Future<Output = ()>>> {
     async {}.boxed_local()
   }
+
+  /// Returns a source map for given `file_name`.
+  ///
+  /// This function will soon be deprecated or renamed.
+  fn get_source_map(&self, _file_name: &str) -> Option<Vec<u8>> {
+    None
+  }
+
+  fn get_source_mapped_source_line(
+    &self,
+    _file_name: &str,
+    _line_number: usize,
+  ) -> Option<String> {
+    None
+  }
 }
 
 /// Placeholder structure used when creating
diff --git a/core/runtime/jsruntime.rs b/core/runtime/jsruntime.rs
index 1081dde..014ddcb 100644
--- a/core/runtime/jsruntime.rs
+++ b/core/runtime/jsruntime.rs
@@ -44,6 +44,7 @@ use crate::runtime::ContextState;
 use crate::runtime::JsRealm;
 use crate::runtime::OpDriverImpl;
 use crate::source_map::SourceMapData;
+#[allow(deprecated)]
 use crate::source_map::SourceMapGetter;
 use crate::source_map::SourceMapper;
 use crate::stats::RuntimeActivityType;
@@ -428,6 +429,8 @@ pub struct JsRuntimeState {
 #[derive(Default)]
 pub struct RuntimeOptions {
   /// Source map reference for errors.
+  #[deprecated = "Update `ModuleLoader` trait implementations. This option will be removed in deno_core v0.300.0."]
+  #[allow(deprecated)]
   pub source_map_getter: Option<Rc<dyn SourceMapGetter>>,
 
   /// Allows to map error type to a string "class" used to represent
@@ -672,7 +675,12 @@ impl JsRuntime {
 
     // Load the sources and source maps
     let mut files_loaded = Vec::with_capacity(128);
-    let mut source_mapper = SourceMapper::new(options.source_map_getter);
+    let loader = options
+      .module_loader
+      .unwrap_or_else(|| Rc::new(NoopModuleLoader));
+    #[allow(deprecated)]
+    let mut source_mapper =
+      SourceMapper::new(loader.clone(), options.source_map_getter);
     let mut sources = extension_set::into_sources(
       options.extension_transpiler.as_deref(),
       &extensions,
@@ -890,9 +898,6 @@ impl JsRuntime {
     // ...now that JavaScript bindings to ops are available we can deserialize
     // modules stored in the snapshot (because they depend on the ops and external
     // references must match properly) and recreate a module map...
-    let loader = options
-      .module_loader
-      .unwrap_or_else(|| Rc::new(NoopModuleLoader));
     let import_meta_resolve_cb = options
       .import_meta_resolve_callback
       .unwrap_or_else(|| Box::new(default_import_meta_resolve_cb));
diff --git a/core/source_map.rs b/core/source_map.rs
index 97e8b4f..2e4e134 100644
--- a/core/source_map.rs
+++ b/core/source_map.rs
@@ -2,13 +2,18 @@
 
 //! This mod provides functions to remap a `JsError` based on a source map.
 
+// TODO(bartlomieju): remove once `SourceMapGetter` is removed.
+#![allow(deprecated)]
+
 use crate::resolve_url;
+use crate::ModuleLoader;
 pub use sourcemap::SourceMap;
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::rc::Rc;
 use std::str;
 
+#[deprecated = "Use `ModuleLoader` methods instead. This trait will be removed in deno_core v0.300.0."]
 pub trait SourceMapGetter {
   /// Returns the raw source map file.
   fn get_source_map(&self, file_name: &str) -> Option<Vec<u8>>;
@@ -40,6 +45,8 @@ pub type SourceMapData = Cow<'static, [u8]>;
 pub struct SourceMapper {
   maps: HashMap<String, Option<SourceMap>>,
   source_lines: HashMap<(String, i64), Option<String>>,
+  loader: Rc<dyn ModuleLoader>,
+
   getter: Option<Rc<dyn SourceMapGetter>>,
   pub(crate) ext_source_maps: HashMap<String, SourceMapData>,
   // This is not the right place for this, but it's the easiest way to make
@@ -48,11 +55,15 @@ pub struct SourceMapper {
 }
 
 impl SourceMapper {
-  pub fn new(getter: Option<Rc<dyn SourceMapGetter>>) -> Self {
+  pub fn new(
+    loader: Rc<dyn ModuleLoader>,
+    getter: Option<Rc<dyn SourceMapGetter>>,
+  ) -> Self {
     Self {
       maps: Default::default(),
       source_lines: Default::default(),
       ext_source_maps: Default::default(),
+      loader,
       getter,
       stashed_file_name: Default::default(),
     }
@@ -84,6 +95,9 @@ impl SourceMapper {
           .or_else(|| {
             SourceMap::from_slice(self.ext_source_maps.get(file_name)?).ok()
           })
+          .or_else(|| {
+            SourceMap::from_slice(&self.loader.get_source_map(file_name)?).ok()
+          })
           .or_else(|| {
             SourceMap::from_slice(&getter?.get_source_map(file_name)?).ok()
           })
@@ -141,13 +155,25 @@ impl SourceMapper {
     line_number: i64,
   ) -> Option<String> {
     let getter = self.getter.as_ref()?;
+
     self
       .source_lines
       .entry((file_name.to_string(), line_number))
       .or_insert_with(|| {
         // Source lookup expects a 0-based line number, ours are 1-based.
-        let s = getter.get_source_line(file_name, (line_number - 1) as usize);
-        s.filter(|s| s.len() <= Self::MAX_SOURCE_LINE_LENGTH)
+        if let Some(source_line) = self
+          .loader
+          .get_source_mapped_source_line(file_name, (line_number - 1) as usize)
+        {
+          if source_line.len() <= Self::MAX_SOURCE_LINE_LENGTH {
+            Some(source_line)
+          } else {
+            None
+          }
+        } else {
+          let s = getter.get_source_line(file_name, (line_number - 1) as usize);
+          s.filter(|s| s.len() <= Self::MAX_SOURCE_LINE_LENGTH)
+        }
       })
       .clone()
   }


Test patch:
diff --git a/testing/checkin/runner/mod.rs b/testing/checkin/runner/mod.rs
index 876c355..98dc529 100644
--- a/testing/checkin/runner/mod.rs
+++ b/testing/checkin/runner/mod.rs
@@ -125,7 +125,6 @@ pub fn create_runtime_from_snapshot(
       deno_core::error::get_custom_error_class(error).unwrap_or("Error")
     }),
     shared_array_buffer_store: Some(CrossIsolateStore::default()),
-    source_map_getter: Some(module_loader),
     custom_module_evaluation_cb: Some(Box::new(custom_module_evaluation_cb)),
     inspector,
     ..Default::default()
diff --git a/testing/checkin/runner/ts_module_loader.rs b/testing/checkin/runner/ts_module_loader.rs
index 2c7e2f6..7534cd8 100644
--- a/testing/checkin/runner/ts_module_loader.rs
+++ b/testing/checkin/runner/ts_module_loader.rs
@@ -28,30 +28,17 @@ use deno_core::ModuleType;
 use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::SourceMapData;
-use deno_core::SourceMapGetter;
 
-#[derive(Clone, Default)]
-struct SourceMapStore(Rc<RefCell<HashMap<String, Vec<u8>>>>);
-
-impl SourceMapGetter for TypescriptModuleLoader {
-  fn get_source_map(&self, specifier: &str) -> Option<Vec<u8>> {
-    self.source_maps.0.borrow().get(specifier).cloned()
-  }
-
-  fn get_source_line(
-    &self,
-    _file_name: &str,
-    _line_number: usize,
-  ) -> Option<String> {
-    None
-  }
-}
+// TODO(bartlomieju): this is duplicated in `core/examples/ts_modules_loader.rs`.
+type SourceMapStore = Rc<RefCell<HashMap<String, Vec<u8>>>>;
 
+// TODO(bartlomieju): this is duplicated in `core/examples/ts_modules_loader.rs`.
 #[derive(Default)]
 pub struct TypescriptModuleLoader {
   source_maps: SourceMapStore,
 }
 
+// TODO(bartlomieju): this is duplicated in `core/examples/ts_modules_loader.rs`.
 impl ModuleLoader for TypescriptModuleLoader {
   fn resolve(
     &self,
@@ -135,7 +122,6 @@ impl ModuleLoader for TypescriptModuleLoader {
         })?;
         let source_map = res.source_map.unwrap();
         source_maps
-          .0
           .borrow_mut()
           .insert(module_specifier.to_string(), source_map.into_bytes());
         res.text
@@ -156,6 +142,10 @@ impl ModuleLoader for TypescriptModuleLoader {
       requested_module_type,
     ))
   }
+
+  fn get_source_map(&self, specifier: &str) -> Option<Vec<u8>> {
+    self.source_maps.borrow().get(specifier).cloned()
+  }
 }
 
 pub fn maybe_transpile_source(