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.

1 files changed, 41 insertions, 12 deletions

diff --git a/cli/module_graph.rs b/cli/module_graph.rs
index e03468679..c63e6848f 100644
--- a/cli/module_graph.rs
+++ b/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,