Skip to content

Commit

Permalink
fix(typescript): don't mix worker mode and linker
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Alex Eagle authored and alexeagle committed Apr 10, 2020
1 parent efe6513 commit 55c6c4a
Show file tree
Hide file tree
Showing 10 changed files with 77 additions and 39 deletions.
6 changes: 3 additions & 3 deletions examples/angular/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/src/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions examples/angular/src/app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
],
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/src/app/hello-world/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/src/app/home/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/src/app/todos/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions examples/angular/src/shared/material/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
24 changes: 12 additions & 12 deletions examples/angular/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
68 changes: 44 additions & 24 deletions packages/typescript/src/internal/build_defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 4 additions & 0 deletions packages/typescript/test/lit_plugin/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 55c6c4a

Please sign in to comment.