From f9e45114b9c423b72e9c44c4a8aef90f5c3b44d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Fri, 22 May 2020 16:01:00 +0200 Subject: [PATCH] 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 | 10 ++++- cli/module_graph.rs | 53 ++++++++++++++++++----- cli/tests/integration_tests.rs | 6 +++ cli/tests/type_directives_redirect.ts | 1 + cli/tests/type_directives_redirect.ts.out | 5 +++ tools/http_server.py | 28 ++++++++++++ 6 files changed, 90 insertions(+), 13 deletions(-) create mode 100644 cli/tests/type_directives_redirect.ts create mode 100644 cli/tests/type_directives_redirect.ts.out diff --git a/cli/js/compiler.ts b/cli/js/compiler.ts index a1189dc2021bd9..06117413a1c2ef 100644 --- a/cli/js/compiler.ts +++ b/cli/js/compiler.ts @@ -585,7 +585,6 @@ function buildSourceFileCache( sourceFileMap: Record ): 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[]; diff --git a/cli/module_graph.rs b/cli/module_graph.rs index e03468679b39b5..c63e6848f83993 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, pub filename: String, pub imports: Vec, pub referenced_files: Vec, @@ -87,13 +89,14 @@ pub struct ModuleGraphFile { } type SourceFileFuture = - Pin>>>; + Pin>>>; pub struct ModuleGraphLoader { permissions: Permissions, file_fetcher: SourceFileFetcher, maybe_import_map: Option, pending_downloads: FuturesUnordered, + has_downloaded: HashSet, 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, ) -> 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, diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index 5419d3270c2f4e..59e5f554da87f6 100644 --- a/cli/tests/integration_tests.rs +++ b/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", diff --git a/cli/tests/type_directives_redirect.ts b/cli/tests/type_directives_redirect.ts new file mode 100644 index 00000000000000..1756d5af9d85a4 --- /dev/null +++ b/cli/tests/type_directives_redirect.ts @@ -0,0 +1 @@ +import "http://localhost:4545/type_directives_redirect.js"; diff --git a/cli/tests/type_directives_redirect.ts.out b/cli/tests/type_directives_redirect.ts.out new file mode 100644 index 00000000000000..7473884f8d4390 --- /dev/null +++ b/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 diff --git a/tools/http_server.py b/tools/http_server.py index e2e9f2d98ecdd2..346b319f801377 100755 --- a/tools/http_server.py +++ b/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')