fix(check): move is cjs check from resolving to loading (#25597)

This is required to do when loading because TypeScript handles and
resolves `/// <reference path="..." />` internally.

2 files changed, 70 insertions, 78 deletions

diff --git a/cli/tsc/99_main_compiler.js b/cli/tsc/99_main_compiler.js
index 42bf1055f..24dda4f17 100644
--- a/cli/tsc/99_main_compiler.js
+++ b/cli/tsc/99_main_compiler.js
@@ -117,27 +117,6 @@ delete Object.prototype.__proto__;
     }
   }
 
-  class SpecifierIsCjsCache {
-    /** @type {Set<string>} */
-    #cache = new Set();
-
-    /** @param {[string, ts.Extension]} param */
-    maybeAdd([specifier, ext]) {
-      if (ext === ".cjs" || ext === ".d.cts" || ext === ".cts") {
-        this.#cache.add(specifier);
-      }
-    }
-
-    add(specifier) {
-      this.#cache.add(specifier);
-    }
-
-    /** @param specifier {string} */
-    has(specifier) {
-      return this.#cache.has(specifier);
-    }
-  }
-
   // In the case of the LSP, this will only ever contain the assets.
   /** @type {Map<string, ts.SourceFile>} */
   const sourceFileCache = new Map();
@@ -154,7 +133,8 @@ delete Object.prototype.__proto__;
   /** @type {Map<string, boolean>} */
   const isNodeSourceFileCache = new Map();
 
-  const isCjsCache = new SpecifierIsCjsCache();
+  /** @type {Map<string, boolean>} */
+  const isCjsCache = new Map();
 
   // Maps asset specifiers to the first scope that the asset was loaded into.
   /** @type {Map<string, string | null>} */
@@ -235,7 +215,7 @@ delete Object.prototype.__proto__;
           scriptSnapshot,
           {
             ...getCreateSourceFileOptions(sourceFileOptions),
-            impliedNodeFormat: isCjsCache.has(fileName)
+            impliedNodeFormat: (isCjsCache.get(fileName) ?? false)
               ? ts.ModuleKind.CommonJS
               : ts.ModuleKind.ESNext,
             // in the lsp we want to be able to show documentation
@@ -636,18 +616,13 @@ delete Object.prototype.__proto__;
       if (!fileInfo) {
         return undefined;
       }
-      let { data, scriptKind, version, isCjs } = fileInfo;
+      const { data, scriptKind, version, isCjs } = fileInfo;
       assert(
         data != null,
         `"data" is unexpectedly null for "${specifier}".`,
       );
 
-      // use the cache for non-lsp
-      if (isCjs == null) {
-        isCjs = isCjsCache.has(specifier);
-      } else if (isCjs) {
-        isCjsCache.add(specifier);
-      }
+      isCjsCache.set(specifier, isCjs);
 
       sourceFile = ts.createSourceFile(
         specifier,
@@ -722,11 +697,10 @@ delete Object.prototype.__proto__;
           /** @type {[string, ts.Extension] | undefined} */
           const resolved = ops.op_resolve(
             containingFilePath,
-            isCjsCache.has(containingFilePath),
+            isCjsCache.get(containingFilePath) ?? false,
             [fileReference.fileName],
           )?.[0];
           if (resolved) {
-            isCjsCache.maybeAdd(resolved);
             return {
               primary: true,
               resolvedFileName: resolved[0],
@@ -756,13 +730,12 @@ delete Object.prototype.__proto__;
       /** @type {Array<[string, ts.Extension] | undefined>} */
       const resolved = ops.op_resolve(
         base,
-        isCjsCache.has(base),
+        isCjsCache.get(base) ?? false,
         specifiers,
       );
       if (resolved) {
         const result = resolved.map((item) => {
           if (item) {
-            isCjsCache.maybeAdd(item);
             const [resolvedFileName, extension] = item;
             return {
               resolvedFileName,
@@ -841,9 +814,7 @@ delete Object.prototype.__proto__;
         if (!fileInfo) {
           return undefined;
         }
-        if (fileInfo.isCjs) {
-          isCjsCache.add(specifier);
-        }
+        isCjsCache.set(specifier, fileInfo.isCjs);
         sourceTextCache.set(specifier, fileInfo.data);
         scriptVersionCache.set(specifier, fileInfo.version);
         sourceText = fileInfo.data;
diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs
index 1b443cafd..6db282f2d 100644
--- a/cli/tsc/mod.rs
+++ b/cli/tsc/mod.rs
@@ -468,6 +468,7 @@ struct LoadResponse {
   data: String,
   version: Option<String>,
   script_kind: i32,
+  is_cjs: bool,
 }
 
 #[op2]
@@ -483,6 +484,29 @@ fn op_load_inner(
   state: &mut OpState,
   load_specifier: &str,
 ) -> Result<Option<LoadResponse>, AnyError> {
+  fn load_from_node_modules(
+    specifier: &ModuleSpecifier,
+    node_resolver: Option<&NodeResolver>,
+    media_type: &mut MediaType,
+    is_cjs: &mut bool,
+  ) -> Result<String, AnyError> {
+    *media_type = MediaType::from_specifier(specifier);
+    *is_cjs = node_resolver
+      .map(|node_resolver| {
+        match node_resolver.url_to_node_resolution(specifier.clone()) {
+          Ok(NodeResolution::CommonJs(_)) => true,
+          Ok(NodeResolution::Esm(_))
+          | Ok(NodeResolution::BuiltIn(_))
+          | Err(_) => false,
+        }
+      })
+      .unwrap_or(false);
+    let file_path = specifier.to_file_path().unwrap();
+    let code = std::fs::read_to_string(&file_path)
+      .with_context(|| format!("Unable to load {}", file_path.display()))?;
+    Ok(code)
+  }
+
   let state = state.borrow_mut::<State>();
 
   let specifier = normalize_specifier(load_specifier, &state.current_dir)
@@ -491,6 +515,7 @@ fn op_load_inner(
   let mut hash: Option<String> = None;
   let mut media_type = MediaType::Unknown;
   let graph = &state.graph;
+  let mut is_cjs = false;
 
   let data = if load_specifier == "internal:///.tsbuildinfo" {
     state.maybe_tsbuildinfo.as_deref().map(Cow::Borrowed)
@@ -522,6 +547,7 @@ fn op_load_inner(
             data: "".to_string(),
             version: Some("1".to_string()),
             script_kind: as_ts_script_kind(*media_type),
+            is_cjs: false,
           }))
         }
         _ => None,
@@ -546,26 +572,25 @@ fn op_load_inner(
           // means it's Deno code importing an npm module
           let specifier =
             node::resolve_specifier_into_node_modules(&module.specifier);
-          media_type = MediaType::from_specifier(&specifier);
-          let file_path = specifier.to_file_path().unwrap();
-          let code =
-            std::fs::read_to_string(&file_path).with_context(|| {
-              format!("Unable to load {}", file_path.display())
-            })?;
-          Some(Cow::Owned(code))
+          Some(Cow::Owned(load_from_node_modules(
+            &specifier,
+            state.maybe_npm.as_ref().map(|n| n.node_resolver.as_ref()),
+            &mut media_type,
+            &mut is_cjs,
+          )?))
         }
       }
-    } else if state
+    } else if let Some(npm) = state
       .maybe_npm
       .as_ref()
-      .map(|npm| npm.node_resolver.in_npm_package(specifier))
-      .unwrap_or(false)
+      .filter(|npm| npm.node_resolver.in_npm_package(specifier))
     {
-      media_type = MediaType::from_specifier(specifier);
-      let file_path = specifier.to_file_path().unwrap();
-      let code = std::fs::read_to_string(&file_path)
-        .with_context(|| format!("Unable to load {}", file_path.display()))?;
-      Some(Cow::Owned(code))
+      Some(Cow::Owned(load_from_node_modules(
+        specifier,
+        Some(npm.node_resolver.as_ref()),
+        &mut media_type,
+        &mut is_cjs,
+      )?))
     } else {
       None
     };
@@ -579,6 +604,7 @@ fn op_load_inner(
     data: data.into_owned(),
     version: hash,
     script_kind: as_ts_script_kind(media_type),
+    is_cjs,
   }))
 }
 
@@ -601,7 +627,7 @@ fn op_resolve(
   #[string] base: String,
   is_base_cjs: bool,
   #[serde] specifiers: Vec<String>,
-) -> Result<Vec<(String, String)>, AnyError> {
+) -> Result<Vec<(String, &'static str)>, AnyError> {
   op_resolve_inner(
     state,
     ResolveArgs {
@@ -616,9 +642,9 @@ fn op_resolve(
 fn op_resolve_inner(
   state: &mut OpState,
   args: ResolveArgs,
-) -> Result<Vec<(String, String)>, AnyError> {
+) -> Result<Vec<(String, &'static str)>, AnyError> {
   let state = state.borrow_mut::<State>();
-  let mut resolved: Vec<(String, String)> =
+  let mut resolved: Vec<(String, &'static str)> =
     Vec::with_capacity(args.specifiers.len());
   let referrer_kind = if args.is_base_cjs {
     NodeModuleKind::Cjs
@@ -640,16 +666,14 @@ fn op_resolve_inner(
     if specifier.starts_with("node:") {
       resolved.push((
         MISSING_DEPENDENCY_SPECIFIER.to_string(),
-        MediaType::Dts.to_string(),
+        MediaType::Dts.as_ts_extension(),
       ));
       continue;
     }
 
     if specifier.starts_with("asset:///") {
-      let media_type = MediaType::from_str(&specifier)
-        .as_ts_extension()
-        .to_string();
-      resolved.push((specifier, media_type));
+      let ext = MediaType::from_str(&specifier).as_ts_extension();
+      resolved.push((specifier, ext));
       continue;
     }
 
@@ -694,11 +718,11 @@ fn op_resolve_inner(
             }
           }
         };
-        (specifier_str, media_type.as_ts_extension().into())
+        (specifier_str, media_type.as_ts_extension())
       }
       None => (
         MISSING_DEPENDENCY_SPECIFIER.to_string(),
-        ".d.ts".to_string(),
+        MediaType::Dts.as_ts_extension(),
       ),
     };
     log::debug!("Resolved {} to {:?}", specifier, result);
@@ -853,14 +877,15 @@ fn resolve_non_graph_specifier_types(
 #[op2(fast)]
 fn op_is_node_file(state: &mut OpState, #[string] path: &str) -> bool {
   let state = state.borrow::<State>();
-  match ModuleSpecifier::parse(path) {
-    Ok(specifier) => state
-      .maybe_npm
-      .as_ref()
-      .map(|n| n.node_resolver.in_npm_package(&specifier))
-      .unwrap_or(false),
-    Err(_) => false,
-  }
+  ModuleSpecifier::parse(path)
+    .ok()
+    .and_then(|specifier| {
+      state
+        .maybe_npm
+        .as_ref()
+        .map(|n| n.node_resolver.in_npm_package(&specifier))
+    })
+    .unwrap_or(false)
 }
 
 #[derive(Debug, Deserialize, Eq, PartialEq)]
@@ -1168,6 +1193,7 @@ mod tests {
         "data": "console.log(\"hello deno\");\n",
         "version": "7821807483407828376",
         "scriptKind": 3,
+        "isCjs": false,
       })
     );
   }
@@ -1206,6 +1232,7 @@ mod tests {
         "data": "some content",
         "version": null,
         "scriptKind": 0,
+        "isCjs": false,
       })
     );
   }
@@ -1235,10 +1262,7 @@ mod tests {
       },
     )
     .expect("should have invoked op");
-    assert_eq!(
-      actual,
-      vec![("https://deno.land/x/b.ts".into(), ".ts".into())]
-    );
+    assert_eq!(actual, vec![("https://deno.land/x/b.ts".into(), ".ts")]);
   }
 
   #[tokio::test]
@@ -1258,10 +1282,7 @@ mod tests {
       },
     )
     .expect("should have not errored");
-    assert_eq!(
-      actual,
-      vec![(MISSING_DEPENDENCY_SPECIFIER.into(), ".d.ts".into())]
-    );
+    assert_eq!(actual, vec![(MISSING_DEPENDENCY_SPECIFIER.into(), ".d.ts")]);
   }
 
   #[tokio::test]