Repository: denoland/deno_core
Pull request #721 (2024-04-26 15:51:22)

refactor: keep ModuleRequest.specifier as a Url
Seems like it was needlessly going back and forth between a Url and String?


Patch:
diff --git a/core/modules/map.rs b/core/modules/map.rs
index fc8101f..773953b 100644
--- a/core/modules/map.rs
+++ b/core/modules/map.rs
@@ -705,7 +705,7 @@ impl ModuleMap {
       let requested_module_type =
         get_requested_module_type_from_attributes(&attributes);
       let request = ModuleRequest {
-        specifier: module_specifier.to_string(),
+        specifier: module_specifier,
         requested_module_type,
       };
       requests.push(request);
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index 1928f18..a05565a 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -589,7 +589,7 @@ impl std::fmt::Display for RequestedModuleType {
 /// which case this will have a `RequestedModuleType::Json`.
 #[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)]
 pub(crate) struct ModuleRequest {
-  pub specifier: String,
+  pub specifier: ModuleSpecifier,
   pub requested_module_type: RequestedModuleType,
 }
 
diff --git a/core/modules/recursive_load.rs b/core/modules/recursive_load.rs
index 42961cf..39778b5 100644
--- a/core/modules/recursive_load.rs
+++ b/core/modules/recursive_load.rs
@@ -231,7 +231,7 @@ impl RecursiveModuleLoad {
     self.visited.insert(module_request.clone());
     while let Some((module_id, module_request)) = already_registered.pop_front()
     {
-      let referrer = ModuleSpecifier::parse(&module_request.specifier).unwrap();
+      let referrer = &module_request.specifier;
       let imports = self
         .module_map_rc
         .get_requested_modules(module_id)
@@ -242,7 +242,7 @@ impl RecursiveModuleLoad {
           && !self
             .visited_as_alias
             .borrow()
-            .contains(&module_request.specifier)
+            .contains(module_request.specifier.as_str())
         {
           if let Some(module_id) = self.module_map_rc.get_id(
             module_request.specifier.as_str(),
@@ -251,8 +251,6 @@ impl RecursiveModuleLoad {
             already_registered.push_back((module_id, module_request.clone()));
           } else {
             let request = module_request.clone();
-            let specifier =
-              ModuleSpecifier::parse(&module_request.specifier).unwrap();
             let visited_as_alias = self.visited_as_alias.clone();
             let referrer = referrer.clone();
             let loader = self.loader.clone();
@@ -263,11 +261,14 @@ impl RecursiveModuleLoad {
               // possible because it can only be populated after completed
               // loads, meaning a duplicate load future may have already been
               // dispatched before we know it's a duplicate.
-              if visited_as_alias.borrow().contains(specifier.as_str()) {
+              if visited_as_alias
+                .borrow()
+                .contains(request.specifier.as_str())
+              {
                 return Ok(None);
               }
               let load_response = loader.load(
-                &specifier,
+                &request.specifier,
                 Some(&referrer),
                 is_dynamic_import,
                 requested_module_type,
@@ -316,7 +317,7 @@ impl Stream for RecursiveModuleLoad {
           _ => RequestedModuleType::None,
         };
         let module_request = ModuleRequest {
-          specifier: module_specifier.to_string(),
+          specifier: module_specifier.clone(),
           requested_module_type: requested_module_type.clone(),
         };
         let load_fut = if let Some(module_id) = inner.root_module_id {


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index 09683c5..33a3996 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -329,11 +329,11 @@ fn test_recursive_load() {
     modules.get_requested_modules(a_id),
     Some(vec![
       ModuleRequest {
-        specifier: "file:///b.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///b.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       },
       ModuleRequest {
-        specifier: "file:///c.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///c.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       },
     ])
@@ -341,14 +341,14 @@ fn test_recursive_load() {
   assert_eq!(
     modules.get_requested_modules(b_id),
     Some(vec![ModuleRequest {
-      specifier: "file:///c.js".to_string(),
+      specifier: ModuleSpecifier::parse("file:///c.js").unwrap(),
       requested_module_type: RequestedModuleType::None,
     },])
   );
   assert_eq!(
     modules.get_requested_modules(c_id),
     Some(vec![ModuleRequest {
-      specifier: "file:///d.js".to_string(),
+      specifier: ModuleSpecifier::parse("file:///d.js").unwrap(),
       requested_module_type: RequestedModuleType::None,
     },])
   );
@@ -415,7 +415,7 @@ fn test_mods() {
     assert_eq!(
       imports,
       Some(vec![ModuleRequest {
-        specifier: "file:///b.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///b.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       },])
     );
@@ -526,7 +526,7 @@ fn test_json_module() {
     assert_eq!(
       imports,
       Some(vec![ModuleRequest {
-        specifier: "file:///c.json".to_string(),
+        specifier: ModuleSpecifier::parse("file:///c.json").unwrap(),
         requested_module_type: RequestedModuleType::Json,
       },])
     );
@@ -552,7 +552,7 @@ fn test_json_module() {
     assert_eq!(
       imports,
       Some(vec![ModuleRequest {
-        specifier: "file:///c.json".to_string(),
+        specifier: ModuleSpecifier::parse("file:///c.json").unwrap(),
         requested_module_type: RequestedModuleType::Json,
       },])
     );
@@ -959,7 +959,7 @@ export const foo = bytes;
       main: true,
       name: "file:///b.png".into_module_name(),
       requests: vec![ModuleRequest {
-        specifier: "file:///b.png".to_string(),
+        specifier: ModuleSpecifier::parse("file:///b.png").unwrap(),
         requested_module_type: RequestedModuleType::Other(
           "foobar-synth".into()
         )
@@ -1139,7 +1139,7 @@ fn test_circular_load() {
     assert_eq!(
       modules.get_requested_modules(circular1_id),
       Some(vec![ModuleRequest {
-        specifier: "file:///circular2.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///circular2.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       }])
     );
@@ -1147,7 +1147,7 @@ fn test_circular_load() {
     assert_eq!(
       modules.get_requested_modules(circular2_id),
       Some(vec![ModuleRequest {
-        specifier: "file:///circular3.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///circular3.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       }])
     );
@@ -1162,11 +1162,11 @@ fn test_circular_load() {
       modules.get_requested_modules(circular3_id),
       Some(vec![
         ModuleRequest {
-          specifier: "file:///circular1.js".to_string(),
+          specifier: ModuleSpecifier::parse("file:///circular1.js").unwrap(),
           requested_module_type: RequestedModuleType::None,
         },
         ModuleRequest {
-          specifier: "file:///circular2.js".to_string(),
+          specifier: ModuleSpecifier::parse("file:///circular2.js").unwrap(),
           requested_module_type: RequestedModuleType::None,
         }
       ])
@@ -1366,11 +1366,11 @@ fn recursive_load_main_with_code() {
     modules.get_requested_modules(main_id),
     Some(vec![
       ModuleRequest {
-        specifier: "file:///b.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///b.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       },
       ModuleRequest {
-        specifier: "file:///c.js".to_string(),
+        specifier: ModuleSpecifier::parse("file:///c.js").unwrap(),
         requested_module_type: RequestedModuleType::None,
       }
     ])
@@ -1378,14 +1378,14 @@ fn recursive_load_main_with_code() {
   assert_eq!(
     modules.get_requested_modules(b_id),
     Some(vec![ModuleRequest {
-      specifier: "file:///c.js".to_string(),
+      specifier: ModuleSpecifier::parse("file:///c.js").unwrap(),
       requested_module_type: RequestedModuleType::None,
     }])
   );
   assert_eq!(
     modules.get_requested_modules(c_id),
     Some(vec![ModuleRequest {
-      specifier: "file:///d.js".to_string(),
+      specifier: ModuleSpecifier::parse("file:///d.js").unwrap(),
       requested_module_type: RequestedModuleType::None,
     }])
   );
diff --git a/core/runtime/tests/snapshot.rs b/core/runtime/tests/snapshot.rs
index 8359a78..a7f0ce4 100644
--- a/core/runtime/tests/snapshot.rs
+++ b/core/runtime/tests/snapshot.rs
@@ -223,7 +223,8 @@ fn es_snapshot() {
       main,
       name: specifier.into(),
       requests: vec![crate::modules::ModuleRequest {
-        specifier: format!("file:///{prev}.js"),
+        specifier: ModuleSpecifier::parse(&format!("file:///{prev}.js"))
+          .unwrap(),
         requested_module_type: RequestedModuleType::None,
       }],
       module_type: ModuleType::JavaScript,