From 4b7d3b060e88c02bc0ca12664f52111a4666b167 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 10 Jun 2020 16:02:41 +0200 Subject: [PATCH] fix: several regressions in TS compiler (#6177) This commit fixes several regressions in TS compiler: * double compilation of same module during same process run * compilation of JavaScript entry point with non-JS imports * unexpected skip of emit during compilation Additional checks were added to ensure "allowJs" setting is used in TS compiler if JavaScript has non-JS dependencies. --- cli/global_state.rs | 165 +++++++++++++++++--- cli/js/compiler.ts | 23 ++- cli/module_graph.rs | 12 +- cli/tests/integration_tests.rs | 6 + cli/tests/single_compile_with_reload.ts | 4 + cli/tests/single_compile_with_reload.ts.out | 5 + cli/tests/ts_import_from_js.deps.js | 2 + cli/tests/ts_import_from_js.js | 4 +- cli/tests/ts_import_from_js.js.out | 2 + cli/tsc.rs | 15 +- 10 files changed, 206 insertions(+), 32 deletions(-) create mode 100644 cli/tests/single_compile_with_reload.ts create mode 100644 cli/tests/single_compile_with_reload.ts.out create mode 100644 cli/tests/ts_import_from_js.deps.js diff --git a/cli/global_state.rs b/cli/global_state.rs index c1ed3af61fff33..44a3946e2946d7 100644 --- a/cli/global_state.rs +++ b/cli/global_state.rs @@ -143,11 +143,13 @@ impl GlobalState { .expect("Source file not found"); // Check if we need to compile files. + let module_graph_files = module_graph.values().collect::>(); let should_compile = needs_compilation( self.ts_compiler.compile_js, out.media_type, - module_graph.values().collect::>(), + &module_graph_files, ); + let allow_js = should_allow_js(&module_graph_files); if should_compile { self @@ -158,6 +160,7 @@ impl GlobalState { target_lib, permissions, module_graph, + allow_js, ) .await?; } @@ -241,6 +244,29 @@ impl GlobalState { } } +/// Determine if TS compiler should be run with `allowJs` setting on. This +/// is the case when there's a JavaScript file with non-JavaScript import. +fn should_allow_js(module_graph_files: &[&ModuleGraphFile]) -> bool { + module_graph_files.iter().any(|module_file| { + if module_file.media_type != (MediaType::JavaScript as i32) { + false + } else { + module_file.imports.iter().any(|import_desc| { + let import_file = module_graph_files + .iter() + .find(|f| { + f.specifier == import_desc.resolved_specifier.to_string().as_str() + }) + .expect("Failed to find imported file"); + let media_type = import_file.media_type; + media_type == (MediaType::TypeScript as i32) + || media_type == (MediaType::TSX as i32) + || media_type == (MediaType::JSX as i32) + }) + } + }) +} + // Compilation happens if either: // - `checkJs` is set to true in TS config // - entry point is a TS file @@ -248,7 +274,7 @@ impl GlobalState { fn needs_compilation( compile_js: bool, media_type: MediaType, - module_graph_files: Vec<&ModuleGraphFile>, + module_graph_files: &[&ModuleGraphFile], ) -> bool { let mut needs_compilation = match media_type { msg::MediaType::TypeScript | msg::MediaType::TSX | msg::MediaType::JSX => { @@ -276,12 +302,49 @@ fn thread_safe() { } #[test] -fn test_needs_compilation() { - assert!(!needs_compilation( - false, - MediaType::JavaScript, - vec![&ModuleGraphFile { - specifier: "some/file.js".to_string(), +fn test_should_allow_js() { + use crate::module_graph::ImportDescriptor; + + assert!(should_allow_js(&[ + &ModuleGraphFile { + specifier: "file:///some/file.ts".to_string(), + url: "file:///some/file.ts".to_string(), + redirect: None, + filename: "some/file.ts".to_string(), + imports: vec![], + referenced_files: vec![], + lib_directives: vec![], + types_directives: vec![], + type_headers: vec![], + media_type: MediaType::TypeScript as i32, + source_code: "function foo() {}".to_string(), + }, + &ModuleGraphFile { + specifier: "file:///some/file1.js".to_string(), + url: "file:///some/file1.js".to_string(), + redirect: None, + filename: "some/file1.js".to_string(), + imports: vec![ImportDescriptor { + specifier: "./file.ts".to_string(), + resolved_specifier: ModuleSpecifier::resolve_url( + "file:///some/file.ts", + ) + .unwrap(), + type_directive: None, + resolved_type_directive: None, + }], + referenced_files: vec![], + lib_directives: vec![], + types_directives: vec![], + type_headers: vec![], + media_type: MediaType::JavaScript as i32, + source_code: "function foo() {}".to_string(), + }, + ],)); + + assert!(!should_allow_js(&[ + &ModuleGraphFile { + specifier: "file:///some/file.js".to_string(), url: "file:///some/file.js".to_string(), redirect: None, filename: "some/file.js".to_string(), @@ -292,28 +355,86 @@ fn test_needs_compilation() { type_headers: vec![], media_type: MediaType::JavaScript as i32, source_code: "function foo() {}".to_string(), - }] - )); - assert!(!needs_compilation(false, MediaType::JavaScript, vec![])); - assert!(needs_compilation(true, MediaType::JavaScript, vec![])); - assert!(needs_compilation(false, MediaType::TypeScript, vec![])); - assert!(needs_compilation(false, MediaType::JSX, vec![])); - assert!(needs_compilation(false, MediaType::TSX, vec![])); - assert!(needs_compilation( + }, + &ModuleGraphFile { + specifier: "file:///some/file1.js".to_string(), + url: "file:///some/file1.js".to_string(), + redirect: None, + filename: "some/file1.js".to_string(), + imports: vec![ImportDescriptor { + specifier: "./file.js".to_string(), + resolved_specifier: ModuleSpecifier::resolve_url( + "file:///some/file.js", + ) + .unwrap(), + type_directive: None, + resolved_type_directive: None, + }], + referenced_files: vec![], + lib_directives: vec![], + types_directives: vec![], + type_headers: vec![], + media_type: MediaType::JavaScript as i32, + source_code: "function foo() {}".to_string(), + }, + ],)); +} + +#[test] +fn test_needs_compilation() { + assert!(!needs_compilation( false, MediaType::JavaScript, - vec![&ModuleGraphFile { - specifier: "some/file.ts".to_string(), - url: "file:///some/file.ts".to_string(), + &[&ModuleGraphFile { + specifier: "some/file.js".to_string(), + url: "file:///some/file.js".to_string(), redirect: None, - filename: "some/file.ts".to_string(), + filename: "some/file.js".to_string(), imports: vec![], referenced_files: vec![], lib_directives: vec![], types_directives: vec![], type_headers: vec![], - media_type: MediaType::TypeScript as i32, + media_type: MediaType::JavaScript as i32, source_code: "function foo() {}".to_string(), - }] + }], + )); + + assert!(!needs_compilation(false, MediaType::JavaScript, &[])); + assert!(needs_compilation(true, MediaType::JavaScript, &[])); + assert!(needs_compilation(false, MediaType::TypeScript, &[])); + assert!(needs_compilation(false, MediaType::JSX, &[])); + assert!(needs_compilation(false, MediaType::TSX, &[])); + assert!(needs_compilation( + false, + MediaType::JavaScript, + &[ + &ModuleGraphFile { + specifier: "file:///some/file.ts".to_string(), + url: "file:///some/file.ts".to_string(), + redirect: None, + filename: "some/file.ts".to_string(), + imports: vec![], + referenced_files: vec![], + lib_directives: vec![], + types_directives: vec![], + type_headers: vec![], + media_type: MediaType::TypeScript as i32, + source_code: "function foo() {}".to_string(), + }, + &ModuleGraphFile { + specifier: "file:///some/file1.js".to_string(), + url: "file:///some/file1.js".to_string(), + redirect: None, + filename: "some/file1.js".to_string(), + imports: vec![], + referenced_files: vec![], + lib_directives: vec![], + types_directives: vec![], + type_headers: vec![], + media_type: MediaType::JavaScript as i32, + source_code: "function foo() {}".to_string(), + }, + ], )); } diff --git a/cli/js/compiler.ts b/cli/js/compiler.ts index 06117413a1c2ef..abe145da2bfe9a 100644 --- a/cli/js/compiler.ts +++ b/cli/js/compiler.ts @@ -1049,6 +1049,7 @@ interface SourceFileMapEntry { interface CompilerRequestCompile { type: CompilerRequestType.Compile; + allowJs: boolean; target: CompilerHostTarget; rootNames: string[]; configPath?: string; @@ -1099,6 +1100,7 @@ interface RuntimeBundleResult { function compile(request: CompilerRequestCompile): CompileResult { const { + allowJs, bundle, config, configPath, @@ -1138,6 +1140,10 @@ function compile(request: CompilerRequestCompile): CompileResult { })); let diagnostics: readonly ts.Diagnostic[] = []; + if (!bundle) { + host.mergeOptions({ allowJs }); + } + // if there is a configuration supplied, we need to parse that if (config && config.length && configPath) { const configResult = host.configure(cwd, configPath, config); @@ -1167,7 +1173,22 @@ function compile(request: CompilerRequestCompile): CompileResult { setRootExports(program, rootNames[0]); } const emitResult = program.emit(); - assert(emitResult.emitSkipped === false, "Unexpected skip of the emit."); + // If `checkJs` is off we still might be compiling entry point JavaScript file + // (if it has `.ts` imports), but it won't be emitted. In that case we skip + // assertion. + if (!bundle) { + if (options.checkJs) { + assert( + emitResult.emitSkipped === false, + "Unexpected skip of the emit." + ); + } + } else { + assert( + emitResult.emitSkipped === false, + "Unexpected skip of the emit." + ); + } // emitResult.diagnostics is `readonly` in TS3.5+ and can't be assigned // without casting. diagnostics = emitResult.diagnostics; diff --git a/cli/module_graph.rs b/cli/module_graph.rs index 519f443ff991d5..54c53c62394d8e 100644 --- a/cli/module_graph.rs +++ b/cli/module_graph.rs @@ -74,22 +74,22 @@ pub struct ModuleGraph(HashMap); #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] pub struct ImportDescriptor { - specifier: String, + pub specifier: String, #[serde(serialize_with = "serialize_module_specifier")] - resolved_specifier: ModuleSpecifier, + pub resolved_specifier: ModuleSpecifier, // These two fields are for support of @deno-types directive // directly prepending import statement - type_directive: Option, + pub type_directive: Option, #[serde(serialize_with = "serialize_option_module_specifier")] - resolved_type_directive: Option, + pub resolved_type_directive: Option, } #[derive(Debug, Serialize)] #[serde(rename_all = "camelCase")] pub struct ReferenceDescriptor { - specifier: String, + pub specifier: String, #[serde(serialize_with = "serialize_module_specifier")] - resolved_specifier: ModuleSpecifier, + pub resolved_specifier: ModuleSpecifier, } #[derive(Debug, Serialize)] diff --git a/cli/tests/integration_tests.rs b/cli/tests/integration_tests.rs index c62a9a5012ce2b..fafce3eb08bfd5 100644 --- a/cli/tests/integration_tests.rs +++ b/cli/tests/integration_tests.rs @@ -1938,6 +1938,12 @@ itest!(cjs_imports { itest!(ts_import_from_js { args: "run --quiet --reload ts_import_from_js.js", output: "ts_import_from_js.js.out", + http_server: true, +}); + +itest!(single_compile_with_reload { + args: "run --reload --allow-read single_compile_with_reload.ts", + output: "single_compile_with_reload.ts.out", }); itest!(proto_exploit { diff --git a/cli/tests/single_compile_with_reload.ts b/cli/tests/single_compile_with_reload.ts new file mode 100644 index 00000000000000..3dd7283664349c --- /dev/null +++ b/cli/tests/single_compile_with_reload.ts @@ -0,0 +1,4 @@ +await import("./005_more_imports.ts"); +console.log("1"); +await import("./005_more_imports.ts"); +console.log("2"); diff --git a/cli/tests/single_compile_with_reload.ts.out b/cli/tests/single_compile_with_reload.ts.out new file mode 100644 index 00000000000000..2cdd71673ddc90 --- /dev/null +++ b/cli/tests/single_compile_with_reload.ts.out @@ -0,0 +1,5 @@ +Compile [WILDCARD]single_compile_with_reload.ts +Compile [WILDCARD]005_more_imports.ts +Hello +1 +2 diff --git a/cli/tests/ts_import_from_js.deps.js b/cli/tests/ts_import_from_js.deps.js new file mode 100644 index 00000000000000..bfea5ac79bf9be --- /dev/null +++ b/cli/tests/ts_import_from_js.deps.js @@ -0,0 +1,2 @@ +import "./005_more_imports.ts"; +export { printHello } from "http://localhost:4545/cli/tests/subdir/mod2.ts"; diff --git a/cli/tests/ts_import_from_js.js b/cli/tests/ts_import_from_js.js index e06ca15a2480a4..f912c27234e4b7 100644 --- a/cli/tests/ts_import_from_js.js +++ b/cli/tests/ts_import_from_js.js @@ -1 +1,3 @@ -import "./005_more_imports.ts"; +import { printHello } from "./ts_import_from_js.deps.js"; +printHello(); +console.log("success"); diff --git a/cli/tests/ts_import_from_js.js.out b/cli/tests/ts_import_from_js.js.out index e965047ad7c578..e1d7a869ff8b11 100644 --- a/cli/tests/ts_import_from_js.js.out +++ b/cli/tests/ts_import_from_js.js.out @@ -1 +1,3 @@ Hello +Hello +success diff --git a/cli/tsc.rs b/cli/tsc.rs index 1b3208e7534965..8d0f0d5ded1592 100644 --- a/cli/tsc.rs +++ b/cli/tsc.rs @@ -405,7 +405,6 @@ impl TsCompiler { }))) } - // TODO(bartlomieju): this method is no longer needed /// Mark given module URL as compiled to avoid multiple compilations of same /// module in single run. fn mark_compiled(&self, url: &Url) { @@ -413,6 +412,11 @@ impl TsCompiler { c.insert(url.clone()); } + fn has_compiled(&self, url: &Url) -> bool { + let c = self.compiled.lock().unwrap(); + c.contains(url) + } + /// Check if there is compiled source in cache that is valid /// and can be used again. // TODO(bartlomieju): there should be check that cached file actually exists @@ -459,10 +463,14 @@ impl TsCompiler { target: TargetLib, permissions: Permissions, module_graph: HashMap, + allow_js: bool, ) -> Result<(), ErrBox> { let mut has_cached_version = false; - if self.use_disk_cache { + // Only use disk cache if `--reload` flag was not used or + // this file has already been compiled during current process + // lifetime. + if self.use_disk_cache || self.has_compiled(&source_file.url) { if let Some(metadata) = self.get_graph_metadata(&source_file.url) { has_cached_version = true; @@ -503,6 +511,7 @@ impl TsCompiler { let j = match (compiler_config.path, compiler_config.content) { (Some(config_path), Some(config_data)) => json!({ "type": msg::CompilerRequestType::Compile as i32, + "allowJs": allow_js, "target": target, "rootNames": root_names, "bundle": bundle, @@ -514,6 +523,7 @@ impl TsCompiler { }), _ => json!({ "type": msg::CompilerRequestType::Compile as i32, + "allowJs": allow_js, "target": target, "rootNames": root_names, "bundle": bundle, @@ -1151,6 +1161,7 @@ mod tests { TargetLib::Main, Permissions::allow_all(), module_graph, + false, ) .await; assert!(result.is_ok());