fix(npm): don't resolve JS files when resolving types (#16854)

Closes #16851

cli/node/mod.rs

@@ -492,7 +492,10 @@ pub fn node_resolve(
       let path = url.to_file_path().unwrap();
       // todo(16370): the module kind is not correct here. I think we need
       // typescript to tell us if the referrer is esm or cjs
-      let path = path_to_declaration_path(path, NodeModuleKind::Esm);
+      let path = match path_to_declaration_path(path, NodeModuleKind::Esm) {
+        Some(path) => path,
+        None => return Ok(None),
+      };
       ModuleSpecifier::from_file_path(path).unwrap()
     }
   };
@@ -532,7 +535,10 @@ pub fn node_resolve_npm_reference(
   let resolved_path = match mode {
     NodeResolutionMode::Execution => resolved_path,
     NodeResolutionMode::Types => {
-      path_to_declaration_path(resolved_path, node_module_kind)
+      match path_to_declaration_path(resolved_path, node_module_kind) {
+        Some(path) => path,
+        None => return Ok(None),
+      }
     }
   };
   let url = ModuleSpecifier::from_file_path(resolved_path).unwrap();
@@ -794,7 +800,9 @@ fn module_resolve(
       // should use the value provided by typescript instead
       let declaration_path =
         path_to_declaration_path(file_path, NodeModuleKind::Esm);
-      Some(ModuleSpecifier::from_file_path(declaration_path).unwrap())
+      declaration_path.map(|declaration_path| {
+        ModuleSpecifier::from_file_path(declaration_path).unwrap()
+      })
     } else {
       Some(resolved_specifier)
     }

cli/tests/npm_tests.rs

@@ -190,19 +190,19 @@ mod npm {
   });
 
   itest!(import_map {
-  args: "run --allow-read --allow-env --import-map npm/import_map/import_map.json npm/import_map/main.js",
-  output: "npm/import_map/main.out",
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-});
+    args: "run --allow-read --allow-env --import-map npm/import_map/import_map.json npm/import_map/main.js",
+    output: "npm/import_map/main.out",
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+  });
 
   itest!(lock_file {
-  args: "run --allow-read --allow-env --lock npm/lock_file/lock.json npm/lock_file/main.js",
-  output: "npm/lock_file/main.out",
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-  exit_code: 10,
-});
+    args: "run --allow-read --allow-env --lock npm/lock_file/lock.json npm/lock_file/main.js",
+    output: "npm/lock_file/main.out",
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+    exit_code: 10,
+  });
 
   itest!(sub_paths {
     args: "run -A --quiet npm/sub_paths/main.jsx",
@@ -296,12 +296,20 @@ mod npm {
   });
 
   itest!(types_ambient_module_import_map {
-  args: "check --quiet --import-map=npm/types_ambient_module/import_map.json npm/types_ambient_module/main_import_map.ts",
-  output: "npm/types_ambient_module/main_import_map.out",
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-  exit_code: 1,
-});
+    args: "check --quiet --import-map=npm/types_ambient_module/import_map.json npm/types_ambient_module/main_import_map.ts",
+    output: "npm/types_ambient_module/main_import_map.out",
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+    exit_code: 1,
+  });
+
+  itest!(no_types_cjs {
+    args: "check --quiet npm/no_types_cjs/main.ts",
+    output_str: Some(""),
+    exit_code: 0,
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+  });
 
   itest!(no_types_in_conditional_exports {
     args: "run --check --unstable npm/no_types_in_conditional_exports/main.ts",
@@ -791,20 +799,20 @@ mod npm {
   }
 
   itest!(compile_errors {
-  args: "compile -A --quiet npm/cached_only/main.ts",
-  output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
-  exit_code: 1,
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-});
+    args: "compile -A --quiet npm/cached_only/main.ts",
+    output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
+    exit_code: 1,
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+  });
 
   itest!(bundle_errors {
-  args: "bundle --quiet npm/esm/main.js",
-  output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
-  exit_code: 1,
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-});
+    args: "bundle --quiet npm/esm/main.js",
+    output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
+    exit_code: 1,
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+  });
 
   itest!(info_chalk_display {
     args: "info --quiet npm/cjs_with_deps/main.js",
@@ -832,14 +840,14 @@ mod npm {
   });
 
   itest!(info_chalk_json_node_modules_dir {
-  args:
-    "info --quiet --node-modules-dir --json $TESTDATA/npm/cjs_with_deps/main.js",
-  output: "npm/cjs_with_deps/main_info_json.out",
-  exit_code: 0,
-  envs: env_vars_for_npm_tests(),
-  http_server: true,
-  temp_cwd: true,
-});
+    args:
+      "info --quiet --node-modules-dir --json $TESTDATA/npm/cjs_with_deps/main.js",
+    output: "npm/cjs_with_deps/main_info_json.out",
+    exit_code: 0,
+    envs: env_vars_for_npm_tests(),
+    http_server: true,
+    temp_cwd: true,
+  });
 
   itest!(info_cli_chalk_display {
     args: "info --quiet npm:chalk@4",

cli/tests/testdata/npm/no_types_cjs/main.ts

@@ -0,0 +1,7 @@
+import mod from "npm:@denotest/no-types-cjs";
+
+// it actually returns a `number` and has that in its
+// jsdocs, but the jsdocs should not have been resolved so
+// this should type check just fine
+const value: string = mod();
+console.log(value);

cli/tests/testdata/npm/registry/@denotest/no-types-cjs/1.0.0/main.js

@@ -0,0 +1,6 @@
+/**
+ * @return {number}
+ */
+ module.exports = function () {
+  return 5;
+};

cli/tests/testdata/npm/registry/@denotest/no-types-cjs/1.0.0/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "@denotest/no-types-cjs",
+  "version": "1.0.0",
+  "main": "./main.js"
+}

ext/node/resolution.rs

@@ -31,7 +31,7 @@ pub enum NodeModuleKind {
 pub fn path_to_declaration_path(
   path: PathBuf,
   referrer_kind: NodeModuleKind,
-) -> PathBuf {
+) -> Option<PathBuf> {
   fn probe_extensions(
     path: &Path,
     referrer_kind: NodeModuleKind,
@@ -56,17 +56,17 @@ pub fn path_to_declaration_path(
     || lowercase_path.ends_with(".d.cts")
     || lowercase_path.ends_with(".d.ts")
   {
-    return path;
+    return Some(path);
   }
   if let Some(path) = probe_extensions(&path, referrer_kind) {
-    return path;
+    return Some(path);
   }
   if path.is_dir() {
     if let Some(path) = probe_extensions(&path.join("index"), referrer_kind) {
-      return path;
+      return Some(path);
     }
   }
-  path
+  None
 }
 
 /// Alternate `PathBuf::with_extension` that will handle known extensions
@@ -743,8 +743,9 @@ pub fn package_resolve(
   let file_path = package_json.path.parent().unwrap().join(&package_subpath);
 
   if conditions == TYPES_CONDITIONS {
-    let declaration_path = path_to_declaration_path(file_path, referrer_kind);
-    Ok(Some(declaration_path))
+    let maybe_declaration_path =
+      path_to_declaration_path(file_path, referrer_kind);
+    Ok(maybe_declaration_path)
   } else {
     Ok(Some(file_path))
   }
@@ -813,8 +814,7 @@ pub fn legacy_main_resolve(
         // a corresponding declaration file
         if let Some(main) = package_json.main(referrer_kind) {
           let main = package_json.path.parent().unwrap().join(main).clean();
-          let path = path_to_declaration_path(main, referrer_kind);
-          if path.exists() {
+          if let Some(path) = path_to_declaration_path(main, referrer_kind) {
             return Ok(Some(path));
           }
         }