Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support --noCheck for parallel transpiling without type-checking #679

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

jbedard
Copy link
Member

@jbedard jbedard commented Aug 29, 2024

Implement the isolated_typecheck parameter and support for type-checking dependent projects in parallel.

When isolated_typecheck is set ts_project assumes tsc can emit the declaration files without the need for declarations from dependencies. This way the only inputs required to tsc are the source files and tsconfig, not dependencies.

Full type-checking is done as a separate isolated action which does depend on all transitive declarations and is only depended on by a test rule.

Close #374
Ref #374, #679

See future improvements to noCheck

Before (backend+frontend don't start until core finishes):
Screenshot 2024-08-30 at 6 22 40 PM

After (core+backend+frontend compile with --noCheck up front, but backend+frontend don't typecheck until core finishes):
Screenshot 2024-08-30 at 6 45 12 PM
Screenshot 2024-08-30 at 6 27 16 PM


Changes are visible to end-users: no

Test plan

  • Covered by existing test cases
  • New test cases added

Copy link

aspect-workflows bot commented Aug 29, 2024

Test

All tests were cache hits

92 tests (100.0%) were fully cached saving 8s.


Buildifier      Format

@jbedard jbedard force-pushed the isolated_declarations branch 5 times, most recently from 02495c3 to 76f4d27 Compare August 29, 2024 21:21
@jbedard jbedard changed the title Isolated declarations feat: transpile using --noCheck when isolatedDeclarations Aug 29, 2024
@jbedard jbedard marked this pull request as ready for review August 29, 2024 21:43
@jbedard
Copy link
Member Author

jbedard commented Aug 29, 2024

Depends on #678

@jbedard jbedard requested a review from alexeagle August 29, 2024 22:18
@jbedard jbedard marked this pull request as draft September 3, 2024 18:42
@jbedard jbedard force-pushed the isolated_declarations branch 6 times, most recently from c7379b9 to 9a4dd5e Compare September 9, 2024 07:00
@jbedard jbedard changed the title feat: transpile using --noCheck when isolatedDeclarations feat: support --noCheck for transpiling without type-checking Sep 9, 2024
@jbedard jbedard force-pushed the isolated_declarations branch 6 times, most recently from c208da5 to f5a680c Compare September 10, 2024 07:27
@jbedard jbedard force-pushed the isolated_declarations branch 6 times, most recently from 76ee41e to 88a4a19 Compare September 21, 2024 10:07
@jbedard jbedard force-pushed the isolated_declarations branch 4 times, most recently from 2068863 to 724716f Compare September 24, 2024 00:44
ts/private/ts_lib.bzl Outdated Show resolved Hide resolved
ts/private/ts_lib.bzl Outdated Show resolved Hide resolved
Copy link
Member

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌮

@jbedard jbedard force-pushed the isolated_declarations branch 4 times, most recently from 24adaf1 to bea58a5 Compare September 24, 2024 01:24
@jbedard jbedard changed the title feat: support --noCheck for transpiling without type-checking feat: support --noCheck for parallel transpiling without type-checking Sep 24, 2024
@jbedard jbedard enabled auto-merge (squash) September 24, 2024 01:30
@jbedard jbedard merged commit 6596a65 into aspect-build:main Sep 24, 2024
23 checks passed
@jbedard jbedard deleted the isolated_declarations branch September 24, 2024 01:30

arguments.add("--noEmit")
typecheck_arguments.add("--noEmit")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When support_workers is True on line 108 we call arguments.use_param_file, in which case I think we'd want "--noEmit" to be added to that params file, rather than a command-line argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh, you mean instead of typecheck_arguments it must be arguments?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, though it seems tricky since arguments is also used down below where we wouldn't want it to have --noEmit. As far as I can tell there's not an easy way to create a copy of an Args object or remove an element. Something like this might be able to help with that: bazelbuild/bazel#6230, but it looks stalled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty dirty, but something like this seems to work

diff --git a/ts/private/ts_project.bzl b/ts/private/ts_project.bzl
index df88bc6..71c9ff3 100644
--- a/ts/private/ts_project.bzl
+++ b/ts/private/ts_project.bzl
@@ -74,7 +74,7 @@ def _ts_project_impl(ctx):
 
     typecheck_outs = []
 
-    arguments = ctx.actions.args()
+    argument_mods = []
     execution_requirements = {}
     executable = ctx.executable.tsc
 
@@ -105,34 +105,35 @@ See https://github.com/aspect-build/rules_ts/issues/361 for more details.
 
     if supports_workers:
         # Set to use a multiline param-file for worker mode
-        arguments.use_param_file("@%s", use_always = True)
-        arguments.set_param_file_format("multiline")
+        argument_mods.append(lambda arguments: arguments.use_param_file("@%s", use_always = True))
+        argument_mods.append(lambda arguments: arguments.set_param_file_format("multiline"))
         execution_requirements["supports-workers"] = "1"
         execution_requirements["worker-key-mnemonic"] = "TsProject"
         executable = ctx.executable.tsc_worker
 
     # Add all arguments from options first to allow users override them via args.
-    arguments.add_all(options.args)
+    argument_mods.append(lambda arguments: arguments.add_all(options.args))
 
     # Add user specified arguments *before* rule supplied arguments
-    arguments.add_all(ctx.attr.args)
+    argument_mods.append(lambda arguments: arguments.add_all(ctx.attr.args))
 
     outdir = _lib.join(
         ctx.label.workspace_root,
         _lib.join(ctx.label.package, ctx.attr.out_dir) if ctx.attr.out_dir else ctx.label.package,
     )
     tsconfig_path = to_output_relative_path(tsconfig)
-    arguments.add_all([
+    argument_mods.append(lambda arguments: arguments.add_all([
         "--project",
         tsconfig_path,
         "--outDir",
         outdir,
         "--rootDir",
         _lib.calculate_root_dir(ctx),
-    ])
+    ]))
+
     if len(typings_outs) > 0:
         declaration_dir = _lib.join(ctx.label.workspace_root, ctx.label.package, typings_out_dir)
-        arguments.add("--declarationDir", declaration_dir)
+        argument_mods.append(lambda arguments:  arguments.add("--declarationDir", declaration_dir))
 
     inputs = srcs_inputs + tsconfig_inputs
 
@@ -156,7 +157,7 @@ See https://github.com/aspect-build/rules_ts/issues/361 for more details.
 
     outputs = js_outs + map_outs + typings_outs + typing_maps_outs
     if ctx.outputs.buildinfo_out:
-        arguments.add("--tsBuildInfoFile", to_output_relative_path(ctx.outputs.buildinfo_out))
+        argument_mods.append(lambda arguments: arguments.add("--tsBuildInfoFile", to_output_relative_path(ctx.outputs.buildinfo_out)))
         outputs.append(ctx.outputs.buildinfo_out)
 
     output_sources = js_outs + map_outs + assets_outs
@@ -222,16 +223,19 @@ See https://github.com/aspect-build/rules_ts/issues/361 for more details.
         typecheck_output = ctx.actions.declare_file(ctx.attr.name + ".typecheck")
         typecheck_outs.append(typecheck_output)
 
-        typecheck_arguments = ctx.actions.args()
-        if supports_workers:
-            typecheck_arguments.add("--bazelValidationFile", typecheck_output.short_path)
+        arguments = ctx.actions.args()
+        for mod in argument_mods:
+            mod(arguments)
 
-        typecheck_arguments.add("--noEmit")
+        arguments.add("--noEmit")
+
+        if supports_workers:
+            arguments.add("--bazelValidationFile", typecheck_output.short_path)
 
         ctx.actions.run(
             executable = executable,
             inputs = transitive_inputs_depset,
-            arguments = [arguments, typecheck_arguments],
+            arguments = [arguments],
             outputs = [typecheck_output],
             mnemonic = "TsProjectCheck",
             execution_requirements = execution_requirements,
@@ -249,6 +253,10 @@ See https://github.com/aspect-build/rules_ts/issues/361 for more details.
         typecheck_outs.extend(output_types)
 
     if use_tsc_for_js or use_tsc_for_dts:
+        arguments = ctx.actions.args()
+        for mod in argument_mods:
+            mod(arguments)
+
         tsc_emit_arguments = ctx.actions.args()
 
         # Type-checking is done async as a separate action and can be skipped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^

const args = fs.readFileSync(p).toString().trim().split('\n');
if (args.includes('--bazelValidationFile')) {

Notably, --bazelValidationFile is also expected to be in the params file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]: type-check dependent projects in parallel
4 participants