chore: flip supports_workers to default False

Fixes #400
aspect-build · Aug 9, 2023 · ca55a5d · ca55a5d
1 parent 5daf9a0
commit ca55a5d
Show file tree

Hide file tree

Showing 7 changed files with 35 additions and 130 deletions.
diff --git a/docs/rules.md b/docs/rules.md
diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md
@@ -67,7 +67,9 @@ redirect the stdout to a file that you can analyze with power tools.
 
 ## Non-deterministic behavior
 
-By default, we run `tsc` in a "watch mode" using the [Bazel Persistent Worker](https://bazel.build/remote/persistent) feature.
+When Worker Support is enabled, we run `tsc` in a "watch mode" using the [Bazel Persistent Worker](https://bazel.build/remote/persistent) feature.
+See the [`supports_workers`](https://docs.aspect.build/rules/aspect_rules_ts/docs/rules#supports_workers-1) attribute for docs on enabling this feature.
+
 However, this risks leaking state from one compilation to another, and you may still encounter such bugs, for example:
 - removing a required types package from `deps` but the compilation still succeeds
 - outputs created by a previous compilation are still produced even though the source file is deleted
@@ -76,8 +78,6 @@ You can confirm that it's a worker bug by running `bazel shutdown` and trying ag
 
 Please check for issues or file one if you find a bug with persistent workers.
 
-To disable persistent workers for a single target, use the `supports_workers` attribute of `ts_project`. To disable globally, add the line `build --strategy=TsProject=sandboxed` to your `.bazelrc`.
-
 ## Which files should be emitted
 
 TypeScript emits for each file in the "Program". `--listFiles` is a `tsc` flag to show what is in the program, and `--listEmittedFiles` shows what was written.

diff --git a/ts/BUILD.bazel b/ts/BUILD.bazel
@@ -65,7 +65,7 @@ bool_flag(
 
 bool_flag(
     name = "supports_workers",
-    build_setting_default = True,
+    build_setting_default = False,
     visibility = ["//visibility:public"],
 )
 
@@ -122,7 +122,6 @@ options(
     ),
     supports_workers = select({
         ":supports_workers_flag": True,
-        # TODO(2.0): default should be False if we set swc as default
         "//conditions:default": False,
     }),
     verbose = select({

diff --git a/ts/defs.bzl b/ts/defs.bzl
@@ -225,19 +225,17 @@ def ts_project(
         ts_build_info_file: The user-specified value of `tsBuildInfoFile` from the tsconfig.
             Helps Bazel to predict the path where the .tsbuildinfo output is written.
 
-        supports_workers: Whether the worker protocol is enabled.
-            To disable worker mode for a particular target set `supports_workers` to `False`.
+        supports_workers: Whether the "Persistent Worker" protocol is enabled, making `tsc` faster.
+            Note that this causes some known correctness bugs, see
+            https://docs.aspect.build/rules/aspect_rules_ts/docs/troubleshooting
 
-            Note that value of this attribute always preferred over `--@aspect_rules_ts//ts:supports_workers` flag
-            unless the `supports_workers` attribute is not set explicitly.
+            Worker mode can be enabled for all `ts_project`s in a build with the
+            `--@aspect_rules_ts//ts:supports_workers` flag.
+            To enable worker mode for all builds in the workspace, add
+            `build --@aspect_rules_ts//ts:supports_workers` to the .bazelrc.
 
-            Worker mode can be disabled workspace wide by using the `--@aspect_rules_ts//ts:supports_workers` flag.
-            To disable worker mode globally, insert `build --@aspect_rules_ts//ts:supports_workers=false` into the .bazelrc.
-
-            Alternatively, worker mode can be controlled via `--strategy`.
-            To disable worker mode globally via `--strategy` insert `build --strategy=TsProject=sandboxed` into the .bazelrc.
-
-            See https://docs.bazel.build/versions/main/user-manual.html#flag--strategy for more details.
+            To enable worker mode for a single target, set `supports_workers` to `1`.
+            To disable worker mode for a single target, set `supports_workers` to `-1`.
 
         **kwargs: passed through to underlying [`ts_project_rule`](#ts_project_rule), eg. `visibility`, `tags`
     """
@@ -391,7 +389,7 @@ def ts_project(
 
     # Disable workers if a custom tsc was provided but not a custom tsc_worker.
     if tsc != _tsc and tsc_worker == _tsc_worker:
-        supports_workers = False
+        supports_workers = -1
 
     ts_project_rule(
         name = tsc_target_name,
@@ -427,6 +425,5 @@ def ts_project(
             "@npm_typescript//:is_typescript_5_or_greater": True,
             "//conditions:default": False,
         }),
-        internal_do_not_depend_supports_workers_is_none = supports_workers == None,
         **kwargs
     )
diff --git a/ts/private/ts_lib.bzl b/ts/private/ts_lib.bzl
@@ -41,19 +41,22 @@ See more details on the `assets` parameter of the `ts_project` macro.
         allow_files = True,
         mandatory = True,
     ),
-    "supports_workers": attr.bool(
-        doc = "Whether the tsc compiler understands Bazel's persistent worker protocol",
-        default = False,
+    "supports_workers": attr.int(
+        doc = """\
+        Whether the tsc compiler understands Bazel's persistent worker protocol.
+
+        Possible values:
+        - 0: use the value of the global --@aspect_rules_ts//ts:supports_workers flag.
+        - 1: Override the global flag, enabling workers for this target.
+        - -1: Override the global flag, disabling workers for this target.
+        """,
+        default = 0,
+        values = [-1, 0, 1],
     ),
     "is_typescript_5_or_greater": attr.bool(
         doc = "Whether TypeScript version is >= 5.0.0",
         default = False,
     ),
-    # TODO(2.0): remove this and make supports_workers a tri-state int.
-    "internal_do_not_depend_supports_workers_is_none": attr.bool(
-        doc = "Internal. DO NOT DEPEND!",
-        default = False,
-    ),
     "transpile": attr.bool(
         doc = "whether tsc should be used to produce .js outputs",
         default = True,

diff --git a/ts/private/ts_project.bzl b/ts/private/ts_project.bzl
@@ -67,13 +67,11 @@ def _ts_project_impl(ctx):
     execution_requirements = {}
     executable = ctx.executable.tsc
 
-    supports_workers = ctx.attr.supports_workers
-
-    # workers can be enabled/disabled globally. if no supports_workers attribute is set explicitly for this target,
-    # which is indicated by internal_do_not_depend_supports_workers_is_none attribute, then set it to global supports_workers config
-    # TODO(2.0): remove this
-    if ctx.attr.internal_do_not_depend_supports_workers_is_none:
-        supports_workers = options.supports_workers
+    supports_workers = options.supports_workers
+    if ctx.attr.supports_workers == 1:
+        supports_workers = True
+    elif ctx.attr.supports_workers == -1:
+        supports_workers = False
 
     host_is_windows = platform_utils.host_platform_is_windows()
     if host_is_windows and supports_workers:

diff --git a/ts/test/flags_test.bzl b/ts/test/flags_test.bzl
@@ -55,46 +55,6 @@ def _find_tsc_action(env, target_under_test):
     asserts.true(env, found_action != None, "could not find the TsProject action")
     return found_action
 
-# explicit supports_workers = true
-def _supports_workers_explicitly_true_test_impl(ctx):
-    env = analysistest.begin(ctx)
-    target_under_test = analysistest.target_under_test(env)
-    action = _find_tsc_action(env, target_under_test)
-    asserts.true(env, action.argv[0].find("npm_typescript/tsc_worker.sh") != -1, "expected workers to be enabled explicitly.")
-    return analysistest.end(env)
-
-_supports_workers_explicitly_true_test = analysistest.make(_supports_workers_explicitly_true_test_impl)
-
-# explicit supports_workers = false
-def _supports_workers_explicitly_false_test_impl(ctx):
-    env = analysistest.begin(ctx)
-    target_under_test = analysistest.target_under_test(env)
-    action = _find_tsc_action(env, target_under_test)
-    asserts.true(env, action.argv[0].find("npm_typescript/tsc.sh") != -1, "expected workers to be disabled explicitly.")
-    return analysistest.end(env)
-
-_supports_workers_explicitly_false_test = analysistest.make(_supports_workers_explicitly_false_test_impl)
-
-# implicit supports_workers = true
-def _supports_workers_implicitly_true_test_impl(ctx):
-    env = analysistest.begin(ctx)
-    target_under_test = analysistest.target_under_test(env)
-    action = _find_tsc_action(env, target_under_test)
-    asserts.true(env, action.argv[0].find("npm_typescript/tsc_worker.sh") != -1, "expected workers to be enabled globally.")
-    return analysistest.end(env)
-
-_supports_workers_implicitly_true_test = analysistest.make(_supports_workers_implicitly_true_test_impl)
-
-# implicit supports_workers = false
-def _supports_workers_implicitly_false_test_impl(ctx):
-    env = analysistest.begin(ctx)
-    target_under_test = analysistest.target_under_test(env)
-    action = _find_tsc_action(env, target_under_test)
-    asserts.true(env, action.argv[0].find("npm_typescript/tsc.sh") != -1, "expected workers to be disabled globally.")
-    return analysistest.end(env)
-
-_supports_workers_implicitly_false_test = analysistest.make(_supports_workers_implicitly_false_test_impl)
-
 # verbose flag = true test
 def _verbose_true_test_impl(ctx):
     env = analysistest.begin(ctx)
@@ -179,52 +139,6 @@ def ts_project_flags_test_suite(name):
         },
     }
 
-    _ts_project_with_flags(
-        name = "supports_workers_explicitly_true",
-        tsconfig = _TSCONFIG,
-        supports_workers = True,
-        # supports_workers should override the flag
-        supports_workers_flag = False,
-    )
-    _supports_workers_explicitly_true_test(
-        name = "supports_workers_explicitly_true_test",
-        target_under_test = ":supports_workers_explicitly_true",
-    )
-
-    _ts_project_with_flags(
-        name = "supports_workers_explicitly_false",
-        tsconfig = _TSCONFIG,
-        supports_workers = False,
-        # supports_workers should override the flag
-        supports_workers_flag = True,
-    )
-    _supports_workers_explicitly_false_test(
-        name = "supports_workers_explicitly_false_test",
-        target_under_test = ":supports_workers_explicitly_false",
-    )
-
-    _ts_project_with_flags(
-        name = "supports_workers_implicitly_true",
-        tsconfig = _TSCONFIG,
-        # supports_workers attribute is not set explicitly so the flag should be respected
-        supports_workers_flag = True,
-    )
-    _supports_workers_implicitly_true_test(
-        name = "supports_workers_implicitly_true_test",
-        target_under_test = ":supports_workers_implicitly_true",
-    )
-
-    _ts_project_with_flags(
-        name = "supports_workers_implicitly_false",
-        tsconfig = _TSCONFIG,
-        # supports_workers attribute is not set explicitly so the flag should be respected
-        supports_workers_flag = False,
-    )
-    _supports_workers_implicitly_false_test(
-        name = "supports_workers_implicitly_false_test",
-        target_under_test = ":supports_workers_implicitly_false",
-    )
-
     _ts_project_with_flags(
         name = "verbose_true",
         tsconfig = _TSCONFIG,
@@ -268,10 +182,6 @@ def ts_project_flags_test_suite(name):
     native.test_suite(
         name = name,
         tests = [
-            ":supports_workers_explicitly_true_test",
-            ":supports_workers_explicitly_false_test",
-            ":supports_workers_implicitly_true_test",
-            ":supports_workers_implicitly_false_test",
             ":verbose_true_test",
             ":verbose_false_test",
             ":skip_lib_check_always_test",