feat: Depcrate SourceMapGetter, move methods to ModuleLoader (#823)

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 #819.

`SourceMapGetter` is scheduled to be removed in 0.297.0.

Author: bartlomieju

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()
   });
 

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;

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

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));

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()
   }

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()

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(