fix: mis-detecting imports on JavaScript when there is no checkJs (#4040)

This PR fixes an issue where we recursively analysed imports on plain JS files
in the compiler irrespective of "checkJs" being true. This caused problems
where when analysing the imports of those files, we would mistake some
import like structures (AMD/CommonJS) as dependencies and try to resolve
the "modules" even though the compiler would not actually look at those files.

cli/js/compiler.ts

@@ -105,13 +105,6 @@ async function tsCompilerOnMessage({
         type: CompilerRequestType[request.type]
       });
 
-      // This will recursively analyse all the code for other imports,
-      // requesting those from the privileged side, populating the in memory
-      // cache which will be used by the host, before resolving.
-      const resolvedRootModules = await processImports(
-        rootNames.map(rootName => [rootName, rootName])
-      );
-
       // When a programme is emitted, TypeScript will call `writeFile` with
       // each file that needs to be emitted.  The Deno compiler host delegates
       // this, to make it easier to perform the right actions, which vary
@@ -141,6 +134,15 @@ async function tsCompilerOnMessage({
         diagnostics = processConfigureResponse(configResult, configPath);
       }
 
+      // This will recursively analyse all the code for other imports,
+      // requesting those from the privileged side, populating the in memory
+      // cache which will be used by the host, before resolving.
+      const resolvedRootModules = await processImports(
+        rootNames.map(rootName => [rootName, rootName]),
+        undefined,
+        host.getCompilationSettings().checkJs
+      );
+
       let emitSkipped = true;
       // if there was a configuration and no diagnostics with it, we will continue
       // to generate the program and possibly emit it.

cli/js/compiler_imports.ts

@@ -120,7 +120,8 @@ function getMediaType(filename: string): MediaType {
 export function processLocalImports(
   sources: Record<string, string>,
   specifiers: Array<[string, string]>,
-  referrer?: string
+  referrer?: string,
+  checkJs = false
 ): string[] {
   if (!specifiers.length) {
     return [];
@@ -143,7 +144,12 @@ export function processLocalImports(
       });
     sourceFile.cache(specifiers[i][0], referrer);
     if (!sourceFile.processed) {
-      processLocalImports(sources, sourceFile.imports(), sourceFile.url);
+      processLocalImports(
+        sources,
+        sourceFile.imports(checkJs),
+        sourceFile.url,
+        checkJs
+      );
     }
   }
   return moduleNames;
@@ -157,7 +163,8 @@ export function processLocalImports(
  * that should be actually resolved. */
 export async function processImports(
   specifiers: Array<[string, string]>,
-  referrer?: string
+  referrer?: string,
+  checkJs = false
 ): Promise<string[]> {
   if (!specifiers.length) {
     return [];
@@ -172,7 +179,11 @@ export async function processImports(
       SourceFile.get(sourceFileJson.url) || new SourceFile(sourceFileJson);
     sourceFile.cache(specifiers[i][0], referrer);
     if (!sourceFile.processed) {
-      await processImports(sourceFile.imports(), sourceFile.url);
+      await processImports(
+        sourceFile.imports(checkJs),
+        sourceFile.url,
+        checkJs
+      );
     }
   }
   return resolvedSources;

cli/js/compiler_sourcefile.ts

@@ -91,7 +91,7 @@ export class SourceFile {
   }
 
   /** Process the imports for the file and return them. */
-  imports(): Array<[string, string]> {
+  imports(checkJs: boolean): Array<[string, string]> {
     if (this.processed) {
       throw new Error("SourceFile has already been processed.");
     }
@@ -102,6 +102,7 @@ export class SourceFile {
       log(`Skipping imports for "${this.filename}"`);
       return [];
     }
+
     const preProcessedFileInfo = ts.preProcessFile(
       this.sourceCode,
       true,
@@ -131,7 +132,13 @@ export class SourceFile {
           getMappedModuleName(importedFile, typeDirectives)
         ]);
       }
-    } else {
+    } else if (
+      !(
+        !checkJs &&
+        (this.mediaType === MediaType.JavaScript ||
+          this.mediaType === MediaType.JSX)
+      )
+    ) {
       process(importedFiles);
     }
     process(referencedFiles);

cli/tests/fix_js_imports.ts

@@ -0,0 +1,3 @@
+import * as amdLike from "./subdir/amd_like.js";
+
+console.log(amdLike);

cli/tests/fix_js_imports.ts.out

@@ -0,0 +1 @@
+{}

cli/tests/integration_tests.rs

@@ -1004,6 +1004,11 @@ itest!(cafile_info {
   http_server: true,
 });
 
+itest!(fix_js_imports {
+  args: "run --reload fix_js_imports.ts",
+  output: "fix_js_imports.ts.out",
+});
+
 #[test]
 fn cafile_fetch() {
   use deno::http_cache::url_to_filename;

cli/tests/subdir/amd_like.js

@@ -0,0 +1,4 @@
+// looks like an AMD module, but isn't
+const define = () => {};
+define(["fake_module"], () => {});
+export {};