fix: redirects handling in module analysis (#5726)

This commit fixes a bug introduced in #5029 that caused bad 
handling of redirects during module analysis. 

Also ensured that duplicate modules are not downloaded.

cli/js/compiler.ts

@@ -585,7 +585,6 @@ function buildSourceFileCache(
   sourceFileMap: Record<string, SourceFileMapEntry>
 ): void {
   for (const entry of Object.values(sourceFileMap)) {
-    assert(entry.sourceCode.length > 0);
     SourceFile.addToCache({
       url: entry.url,
       filename: entry.url,
@@ -596,7 +595,15 @@ function buildSourceFileCache(
     for (const importDesc of entry.imports) {
       let mappedUrl = importDesc.resolvedSpecifier;
       const importedFile = sourceFileMap[importDesc.resolvedSpecifier];
+      // IMPORTANT: due to HTTP redirects we might end up in situation
+      // where URL points to a file with completely different URL.
+      // In that case we take value of `redirect` field and cache
+      // resolved specifier pointing to the value of the redirect.
+      // It's not very elegant solution and should be rethinked.
       assert(importedFile);
+      if (importedFile.redirect) {
+        mappedUrl = importedFile.redirect;
+      }
       const isJsOrJsx =
         importedFile.mediaType === MediaType.JavaScript ||
         importedFile.mediaType === MediaType.JSX;
@@ -1032,6 +1039,7 @@ interface SourceFileMapEntry {
   url: string;
   sourceCode: string;
   mediaType: MediaType;
+  redirect?: string;
   imports: ImportDescriptor[];
   referencedFiles: ReferenceDescriptor[];
   libDirectives: ReferenceDescriptor[];

cli/module_graph.rs

@@ -19,6 +19,7 @@ use futures::FutureExt;
 use serde::Serialize;
 use serde::Serializer;
 use std::collections::HashMap;
+use std::collections::HashSet;
 use std::hash::BuildHasher;
 use std::path::PathBuf;
 use std::pin::Pin;
@@ -76,6 +77,7 @@ pub struct ReferenceDescriptor {
 pub struct ModuleGraphFile {
   pub specifier: String,
   pub url: String,
+  pub redirect: Option<String>,
   pub filename: String,
   pub imports: Vec<ImportDescriptor>,
   pub referenced_files: Vec<ReferenceDescriptor>,
@@ -87,13 +89,14 @@ pub struct ModuleGraphFile {
 }
 
 type SourceFileFuture =
-  Pin<Box<dyn Future<Output = Result<SourceFile, ErrBox>>>>;
+  Pin<Box<dyn Future<Output = Result<(ModuleSpecifier, SourceFile), ErrBox>>>>;
 
 pub struct ModuleGraphLoader {
   permissions: Permissions,
   file_fetcher: SourceFileFetcher,
   maybe_import_map: Option<ImportMap>,
   pending_downloads: FuturesUnordered<SourceFileFuture>,
+  has_downloaded: HashSet<ModuleSpecifier>,
   pub graph: ModuleGraph,
   is_dyn_import: bool,
   analyze_dynamic_imports: bool,
@@ -112,6 +115,7 @@ impl ModuleGraphLoader {
       permissions,
       maybe_import_map,
       pending_downloads: FuturesUnordered::new(),
+      has_downloaded: HashSet::new(),
       graph: ModuleGraph(HashMap::new()),
       is_dyn_import,
       analyze_dynamic_imports,
@@ -131,8 +135,9 @@ impl ModuleGraphLoader {
     self.download_module(specifier.clone(), None)?;
 
     loop {
-      let source_file = self.pending_downloads.next().await.unwrap()?;
-      self.visit_module(&source_file.url.clone().into(), source_file)?;
+      let (specifier, source_file) =
+        self.pending_downloads.next().await.unwrap()?;
+      self.visit_module(&specifier, source_file)?;
       if self.pending_downloads.is_empty() {
         break;
       }
@@ -260,6 +265,7 @@ impl ModuleGraphLoader {
       ModuleGraphFile {
         specifier: specifier.to_string(),
         url: specifier.to_string(),
+        redirect: None,
         media_type: map_file_extension(&PathBuf::from(specifier.clone()))
           as i32,
         filename: specifier,
@@ -281,7 +287,7 @@ impl ModuleGraphLoader {
     module_specifier: ModuleSpecifier,
     maybe_referrer: Option<ModuleSpecifier>,
   ) -> Result<(), ErrBox> {
-    if self.graph.0.contains_key(&module_specifier.to_string()) {
+    if self.has_downloaded.contains(&module_specifier) {
       return Ok(());
     }
 
@@ -319,6 +325,7 @@ impl ModuleGraphLoader {
       }
     }
 
+    self.has_downloaded.insert(module_specifier.clone());
     let spec = module_specifier;
     let file_fetcher = self.file_fetcher.clone();
     let perms = self.permissions.clone();
@@ -328,13 +335,7 @@ impl ModuleGraphLoader {
       let source_file = file_fetcher
         .fetch_source_file(&spec_, maybe_referrer, perms)
         .await?;
-      // FIXME(bartlomieju):
-      // because of redirects we may end up with wrong URL,
-      // substitute with original one
-      Ok(SourceFile {
-        url: spec_.as_url().to_owned(),
-        ..source_file
-      })
+      Ok((spec_.clone(), source_file))
     }
     .boxed_local();
 
@@ -353,6 +354,33 @@ impl ModuleGraphLoader {
     let mut types_directives = vec![];
     let mut type_headers = vec![];
 
+    // IMPORTANT: source_file.url might be different than requested
+    // module_specifier because of HTTP redirects. In such
+    // situation we add an "empty" ModuleGraphFile with 'redirect'
+    // field set that will be later used in TS worker when building
+    // map of available source file. It will perform substitution
+    // for proper URL point to redirect target.
+    if module_specifier.as_url() != &source_file.url {
+      // TODO(bartlomieju): refactor, this is a band-aid
+      self.graph.0.insert(
+        module_specifier.to_string(),
+        ModuleGraphFile {
+          specifier: module_specifier.to_string(),
+          url: module_specifier.to_string(),
+          redirect: Some(source_file.url.to_string()),
+          filename: source_file.filename.to_str().unwrap().to_string(),
+          media_type: source_file.media_type as i32,
+          source_code: "".to_string(),
+          imports: vec![],
+          referenced_files: vec![],
+          lib_directives: vec![],
+          types_directives: vec![],
+          type_headers: vec![],
+        },
+      );
+    }
+
+    let module_specifier = ModuleSpecifier::from(source_file.url.clone());
     let source_code = String::from_utf8(source_file.source_code)?;
 
     if source_file.media_type == MediaType::JavaScript
@@ -470,7 +498,8 @@ impl ModuleGraphLoader {
       module_specifier.to_string(),
       ModuleGraphFile {
         specifier: module_specifier.to_string(),
-        url: source_file.url.to_string(),
+        url: module_specifier.to_string(),
+        redirect: None,
         filename: source_file.filename.to_str().unwrap().to_string(),
         media_type: source_file.media_type as i32,
         source_code,

cli/tests/integration_tests.rs

@@ -1565,6 +1565,12 @@ itest!(type_directives_js_main {
   exit_code: 0,
 });
 
+itest!(type_directives_redirect {
+  args: "run --reload type_directives_redirect.ts",
+  output: "type_directives_redirect.ts.out",
+  http_server: true,
+});
+
 itest!(types {
   args: "types",
   output: "types.out",

cli/tests/type_directives_redirect.ts

@@ -0,0 +1 @@
+import "http://localhost:4545/type_directives_redirect.js";

cli/tests/type_directives_redirect.ts.out

@@ -0,0 +1,5 @@
+Download [WILDCARD]type_directives_redirect.js
+Download [WILDCARD]xTypeScriptTypesRedirect.d.ts
+Download [WILDCARD]xTypeScriptTypesRedirect.d.ts
+Download [WILDCARD]xTypeScriptTypesRedirected.d.ts
+Compile [WILDCARD]type_directives_redirect.ts

tools/http_server.py

@@ -122,6 +122,34 @@ def do_GET(self):
             self.wfile.write(bytes("export const foo = 'foo';"))
             return
 
+        if "type_directives_redirect.js" in self.path:
+            self.protocol_version = "HTTP/1.1"
+            self.send_response(200, 'OK')
+            self.send_header('Content-type', 'application/javascript')
+            self.send_header(
+                'X-TypeScript-Types',
+                'http://localhost:4547/xTypeScriptTypesRedirect.d.ts')
+            self.end_headers()
+            self.wfile.write(bytes("export const foo = 'foo';"))
+            return
+
+        if "xTypeScriptTypesRedirect.d.ts" in self.path:
+            self.protocol_version = "HTTP/1.1"
+            self.send_response(200, 'OK')
+            self.send_header('Content-type', 'application/typescript')
+            self.end_headers()
+            self.wfile.write(
+                bytes("import './xTypeScriptTypesRedirected.d.ts';"))
+            return
+
+        if "xTypeScriptTypesRedirected.d.ts" in self.path:
+            self.protocol_version = "HTTP/1.1"
+            self.send_response(200, 'OK')
+            self.send_header('Content-type', 'application/typescript')
+            self.end_headers()
+            self.wfile.write(bytes("export const foo: 'foo';"))
+            return
+
         if "xTypeScriptTypes.d.ts" in self.path:
             self.protocol_version = "HTTP/1.1"
             self.send_response(200, 'OK')