-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
02495c3
to
76f4d27
Compare
Depends on #678 |
76f4d27
to
193d2f2
Compare
193d2f2
to
a197c93
Compare
c7379b9
to
9a4dd5e
Compare
c208da5
to
f5a680c
Compare
76ee41e
to
88a4a19
Compare
2068863
to
724716f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮
24adaf1
to
bea58a5
Compare
bea58a5
to
559519b
Compare
|
||
arguments.add("--noEmit") | ||
typecheck_arguments.add("--noEmit") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
rules_ts/ts/private/ts_project_worker.js
Lines 997 to 999 in 4a4bf96
const args = fs.readFileSync(p).toString().trim().split('\n'); | |
if (args.includes('--bazelValidationFile')) { |
Notably, --bazelValidationFile
is also expected to be in the params file
Implement the
isolated_typecheck
parameter and support for type-checking dependent projects in parallel.When
isolated_typecheck
is setts_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):
After (core+backend+frontend compile with
--noCheck
up front, but backend+frontend don't typecheck until core finishes):Changes are visible to end-users: no
Test plan