From 55c6c4ac625b2f30ff827f0ac9f63e01c95653e0 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Wed, 8 Apr 2020 22:47:09 -0700 Subject: [PATCH] fix(typescript): don't mix worker mode and linker This causes hard-to-reproduce resource leaks as demonstrated in #1803 Also we didn't fully understand this before, for example if a worker process stayed running we'd never re-link under it when deps change For now make the conservative fix to keep these two capabilities distinct Fixes #1803 --- examples/angular/package.json | 6 +- examples/angular/src/BUILD.bazel | 2 + examples/angular/src/app/BUILD.bazel | 4 ++ .../angular/src/app/hello-world/BUILD.bazel | 2 + examples/angular/src/app/home/BUILD.bazel | 2 + examples/angular/src/app/todos/BUILD.bazel | 2 + .../angular/src/shared/material/BUILD.bazel | 2 + examples/angular/yarn.lock | 24 +++---- .../typescript/src/internal/build_defs.bzl | 68 ++++++++++++------- .../typescript/test/lit_plugin/BUILD.bazel | 4 ++ 10 files changed, 77 insertions(+), 39 deletions(-) diff --git a/examples/angular/package.json b/examples/angular/package.json index c4c0e84c9e..ab28910a0f 100644 --- a/examples/angular/package.json +++ b/examples/angular/package.json @@ -9,11 +9,11 @@ }, "dependencies": { "@angular/animations": "9.1.0", - "@angular/cdk": "9.0.0", + "@angular/cdk": "9.2.0", "@angular/common": "9.1.0", "@angular/core": "9.1.0", "@angular/forms": "9.1.0", - "@angular/material": "9.0.0", + "@angular/material": "9.2.0", "@angular/platform-browser": "9.1.0", "@angular/platform-browser-dynamic": "9.1.0", "@angular/platform-server": "^9.1.0", @@ -61,7 +61,7 @@ "rollup-plugin-commonjs": "^10.1.0", "rollup-plugin-node-resolve": "^5.2.0", "terser": "4.3.1", - "typescript": "3.6.4" + "typescript": "3.8.3" }, "scripts": { "build": "bazelisk build //src:prodapp", diff --git a/examples/angular/src/BUILD.bazel b/examples/angular/src/BUILD.bazel index 54b1397f28..b3b69d960b 100644 --- a/examples/angular/src/BUILD.bazel +++ b/examples/angular/src/BUILD.bazel @@ -53,6 +53,8 @@ ts_library( use_angular_plugin = True, deps = [ "//src/app", + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/platform-browser", "@npm//@angular/router", diff --git a/examples/angular/src/app/BUILD.bazel b/examples/angular/src/app/BUILD.bazel index 4196ddb98d..e7e9d07fef 100644 --- a/examples/angular/src/app/BUILD.bazel +++ b/examples/angular/src/app/BUILD.bazel @@ -17,6 +17,8 @@ ts_library( "//src/app/todos", "//src/app/todos/reducers", "//src/shared/material", + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/platform-browser", "@npm//@angular/router", @@ -31,6 +33,8 @@ ts_library( use_angular_plugin = True, deps = [ ":app", + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/platform-server", ], diff --git a/examples/angular/src/app/hello-world/BUILD.bazel b/examples/angular/src/app/hello-world/BUILD.bazel index cef9669d86..69653d2c32 100644 --- a/examples/angular/src/app/hello-world/BUILD.bazel +++ b/examples/angular/src/app/hello-world/BUILD.bazel @@ -24,6 +24,8 @@ ts_library( deps = [ "//src/lib/shorten", "//src/shared/material", + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/forms", "@npm//@angular/router", diff --git a/examples/angular/src/app/home/BUILD.bazel b/examples/angular/src/app/home/BUILD.bazel index 32fd6247c9..665d63a2be 100644 --- a/examples/angular/src/app/home/BUILD.bazel +++ b/examples/angular/src/app/home/BUILD.bazel @@ -9,6 +9,8 @@ ts_library( tsconfig = "//src:tsconfig.json", use_angular_plugin = True, deps = [ + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/router", ], diff --git a/examples/angular/src/app/todos/BUILD.bazel b/examples/angular/src/app/todos/BUILD.bazel index 116289a571..503c134ee3 100644 --- a/examples/angular/src/app/todos/BUILD.bazel +++ b/examples/angular/src/app/todos/BUILD.bazel @@ -23,6 +23,8 @@ ts_library( deps = [ "//src/app/todos/reducers", "//src/shared/material", + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/common", "@npm//@angular/core", "@npm//@angular/forms", diff --git a/examples/angular/src/shared/material/BUILD.bazel b/examples/angular/src/shared/material/BUILD.bazel index 313dbfe3e6..92ddc4f664 100644 --- a/examples/angular/src/shared/material/BUILD.bazel +++ b/examples/angular/src/shared/material/BUILD.bazel @@ -8,6 +8,8 @@ ts_library( tsconfig = "//src:tsconfig.json", use_angular_plugin = True, deps = [ + # Needed for the angular compiler plugin + "@npm//@angular/compiler-cli", "@npm//@angular/core", "@npm//@angular/material", ], diff --git a/examples/angular/yarn.lock b/examples/angular/yarn.lock index 8a81317214..228ac1d569 100644 --- a/examples/angular/yarn.lock +++ b/examples/angular/yarn.lock @@ -44,10 +44,10 @@ shelljs "0.8.2" tsickle "^0.38.0" -"@angular/cdk@9.0.0": - version "9.0.0" - resolved "https://registry.yarnpkg.com/@angular/cdk/-/cdk-9.0.0.tgz#5734817ae97044f90d304fa0f25c9c1a7fa0bf96" - integrity sha512-2kYpyYbewIB6fubSIDMvSprJLNplRZoL/AtXW3od4dLyRxtzX+7iWTAtzUG/dhq8CKev0lpd1HENh5lLR/Lhjw== +"@angular/cdk@9.2.0": + version "9.2.0" + resolved "https://registry.yarnpkg.com/@angular/cdk/-/cdk-9.2.0.tgz#587e4a9d5046fa89a68d8eddaee6b185e2915842" + integrity sha512-jeeznvNDpR9POuxzz8Y0zFvMynG9HCJo3ZPTqOjlOq8Lj8876+rLsHDvKEMeLdwlkdi1EweYJW1CLQzI+TwqDA== optionalDependencies: parse5 "^5.0.0" @@ -115,10 +115,10 @@ resolved "https://registry.yarnpkg.com/@angular/forms/-/forms-9.1.0.tgz#de14e34aa37bd41a28f93fee8666cd7f6393078c" integrity sha512-5GC8HQlPChPV+168zLlm4yj4syA6N9ChSKV0tmzj1zIfMcub1UAOaB9IYaXRHQsjPFh9OuQXwmkzScyAfhEVjA== -"@angular/material@9.0.0": - version "9.0.0" - resolved "https://registry.yarnpkg.com/@angular/material/-/material-9.0.0.tgz#655bfd4d4047337e84480b9f92be8e81af375b92" - integrity sha512-QxN2rmR5mvg2YE1NoIGWLpbnmcJq0iFidzy6odzvN17+XkoCJBZ65IdYsHrJgfwGpoIy6bywuixrDHHcSh9I5w== +"@angular/material@9.2.0": + version "9.2.0" + resolved "https://registry.yarnpkg.com/@angular/material/-/material-9.2.0.tgz#1b6f0a2e115f93885d7fc2dc4b258d8c9cf6821f" + integrity sha512-KKzEIVh6/m56m+Ao8p4PK0SyEr0574l3VP2swj1qPag3u+FYgemmXCGTaChrKdDsez+zeTCPXImBGXzE6NQ80Q== "@angular/platform-browser-dynamic@9.1.0": version "9.1.0" @@ -6907,10 +6907,10 @@ typedarray@^0.0.6: resolved "https://registry.yarnpkg.com/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777" integrity sha1-hnrHTjhkGHsdPUfZlqeOxciDB3c= -typescript@3.6.4: - version "3.6.4" - resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.6.4.tgz#b18752bb3792bc1a0281335f7f6ebf1bbfc5b91d" - integrity sha512-unoCll1+l+YK4i4F8f22TaNVPRHcD9PA3yCuZ8g5e0qGqlVlJ/8FSateOLLSagn+Yg5+ZwuPkL8LFUc0Jcvksg== +typescript@3.8.3: + version "3.8.3" + resolved "https://registry.yarnpkg.com/typescript/-/typescript-3.8.3.tgz#409eb8544ea0335711205869ec458ab109ee1061" + integrity sha512-MYlEfn5VrLNsgudQTVJeNaQFUAI7DkhnOjdpAp4T+ku1TfQClewlbSuTVHiA+8skNBgaf02TL/kLOvig4y3G8w== typescript@~3.5.3: version "3.5.3" diff --git a/packages/typescript/src/internal/build_defs.bzl b/packages/typescript/src/internal/build_defs.bzl index c86ca69f1c..ea408aa773 100644 --- a/packages/typescript/src/internal/build_defs.bzl +++ b/packages/typescript/src/internal/build_defs.bzl @@ -140,34 +140,54 @@ def _compile_action(ctx, inputs, outputs, tsconfig_file, node_opts, description # Pass actual options for the node binary in the special "--node_options" argument. arguments = ["--node_options=%s" % opt for opt in node_opts] - # One at-sign makes this a params-file, enabling the worker strategy. - # Two at-signs escapes the argument so it's passed through to tsc_wrapped - # rather than the contents getting expanded. - if ctx.attr.supports_workers: + # We don't try to use the linker to launch the worker process + # because it causes bazel to spawn a new worker for every action + # See https://github.com/bazelbuild/rules_nodejs/issues/1803 + # TODO: understand the interaction between linker and workers better + # When using plugins, we need the linker, so we disable workers for that case as well + if ctx.attr.supports_workers and not ctx.attr.use_angular_plugin: + # One at-sign makes this a params-file, enabling the worker strategy. + # Two at-signs escapes the argument so it's passed through to tsc_wrapped + # rather than the contents getting expanded. arguments.append("@@" + tsconfig_file.path) - mnemonic = "TypeScriptCompile" + + # Spawn a plain action that runs worker process with no linker + ctx.actions.run( + progress_message = "Compiling TypeScript (%s) %s" % (description, ctx.label), + mnemonic = "TypeScriptCompile", + inputs = action_inputs, + outputs = action_outputs, + # Use the built-in shell environment + # Allow for users who set a custom shell that can locate standard binaries like tr and uname + # See https://github.com/NixOS/nixpkgs/issues/43955#issuecomment-407546331 + use_default_shell_env = True, + arguments = arguments, + executable = ctx.executable.compiler, + execution_requirements = { + "supports-workers": "1", + }, + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, + ) else: - arguments.append("-p") + # TODO: if compiler is vanilla tsc, then we need the '-p' argument too + # arguments.append("-p") arguments.append(tsconfig_file.path) - mnemonic = "tsc" - run_node( - ctx, - progress_message = "Compiling TypeScript (%s) %s" % (description, ctx.label), - mnemonic = mnemonic, - inputs = action_inputs, - outputs = action_outputs, - # Use the built-in shell environment - # Allow for users who set a custom shell that can locate standard binaries like tr and uname - # See https://github.com/NixOS/nixpkgs/issues/43955#issuecomment-407546331 - use_default_shell_env = True, - arguments = arguments, - executable = "compiler", - execution_requirements = { - "supports-workers": str(int(ctx.attr.supports_workers)), - }, - env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, - ) + # Run with linker but not as a worker process + run_node( + ctx, + progress_message = "Compiling TypeScript (%s) %s" % (description, ctx.label), + mnemonic = "tsc", + inputs = action_inputs, + outputs = action_outputs, + # Use the built-in shell environment + # Allow for users who set a custom shell that can locate standard binaries like tr and uname + # See https://github.com/NixOS/nixpkgs/issues/43955#issuecomment-407546331 + use_default_shell_env = True, + arguments = arguments, + executable = "compiler", + env = {"COMPILATION_MODE": ctx.var["COMPILATION_MODE"]}, + ) # Enable the replay_params in case an aspect needs to re-build this library. return struct( diff --git a/packages/typescript/test/lit_plugin/BUILD.bazel b/packages/typescript/test/lit_plugin/BUILD.bazel index 0323796722..1bd9a7cbe3 100644 --- a/packages/typescript/test/lit_plugin/BUILD.bazel +++ b/packages/typescript/test/lit_plugin/BUILD.bazel @@ -26,6 +26,10 @@ ts_library( "TS2322: \\[lit\\] Type '444' is not assignable to 'string", "TS2322: \\[lit\\] Type '{ field: number; }' is not assignable to '{ field: string; }'", ], + # We need the linker to set up a node_modules tree to discover the plugin + # but it isn't compatible with worker mode; + # see https://github.com/bazelbuild/rules_nodejs/issues/1803 + supports_workers = False, tsconfig = ":tsconfig.json", deps = [ "@npm//lit-element",