-
Notifications
You must be signed in to change notification settings - Fork 522
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(typescript): allow alternative transpilers #3134
Conversation
4dec83d
to
83ae1e6
Compare
3a366ae
to
92df30b
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.
🤌🏻
|
||
The rules_nodejs authors believe that [SWC](https://swc.rs) is a great choice. | ||
|
||
transpiler_args: if the `transpiler` attribute is a rule or macro, then this list |
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.
what about making this a dict to support arbitrary transpiler attrs such as swcrc
?
transpiler_attrs = {
"args": [...],
"swcrc": "...",
}
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.
you can check the type of a param in a macro so in theory you could overload transpiler_args
to support either a list (passed to args) or a dict (to support multiple attrs)
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.
I did think about swcrc specifically, and a common case could be to pass it as a config setting bazel build --@aspect_rules_swc//:rc=.swcrc
so it applies as the default for your whole build.
But of course someone will have the use case of different swcrc for two different folders of code, so yes I could see this being needed.
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.
People who are using this attributes are most likely going to write a wrapper with defaults. Can't they just write a wrapper around swc that is passed to ts project which invokes swc with defaults? Does this sound too complex?
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.
just changed it to transpiler_kwargs for simplicity
d4e6099
to
c9cfc1a
Compare
docs/TypeScript.md
Outdated
to a better support contract. | ||
|
||
Instead, you can pass a rule or macro that accepts these arguments: | ||
`(name, srcs, js_outs, map_outs, args, data, tags, visibility)` |
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.
nit: update doc string to reflect that testonly
is also passed
tags = kwargs.get("tags", []), | ||
visibility = kwargs.get("visibility", None), | ||
testonly = kwargs.get("testonly", None), | ||
**transpiler_kwargs |
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.
This is getting into the weeds but if you wanted to pass different tags
to the transpiler then you could in theory write
ts_project(
name = "foo",
tags = ["foo"],
transpiler_kwargs = {
tags: ["bar"],
}
and this could be supported if kwargs.get("tags", [])
was merged into transpiler_kwargs
if and only if transpiler_kwargs
does not already have a tags
.
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.
Complete list of "common" attributes to automatically merge into transpiler_kwargs
is https://docs.bazel.build/versions/main/be/common-definitions.html#common-attributes
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.
once we are passing through so much to the transpiler, it starts feeling like this macro is too leaky, and the user should just write their pair of typecheck(emitDeclarationOnly=true) and transpile() rules in their BUILD file.
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.
How about instead of hiding this complexity inside ts_project, we expect users to write a simple wrapper and pass it to the transpiler? in the case of .swcrc
i believe a macro like below can be written once and reused everywhere.
def swc_with_swc_rc(**kwargs):
return swc(swcrc = "//.swcrc", **kwargs)
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.
@tjgq I wonder if you have thoughts about this. I think we discussed the idea once?
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.
oh @thesayyn you can use https://github.com/bazelbuild/bazel-skylib/blob/main/lib/partial.bzl for this, so that the swcrc argument is bound where you make the higher-order function, then ts_project calls that partial.
(Bazel 5 will be nicer here since you could just use a lambda)
PTAL
to a better support contract. | ||
|
||
Instead, you can pass a rule or macro that accepts these arguments: | ||
`(name, srcs, js_outs, map_outs, args, data, tags, visibility)` |
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.
Upon my experimentation using our own swc rules, I realized that this won’t work out of the box. macro does not accept many of these attributes. and some of these attributes get declared twice.
https://github.com/aspect-build/rules_swc/blob/33d5a39030cb87b8d5bc2f03473429d49ab8657b/swc/swc.bzl#L38
so you end up having an error that you don’t know what to do with.
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.
Right, the swc_rule is what exposes this API
346dab4
to
a76ab93
Compare
a76ab93
to
39162b6
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.
using a dict is a really clever trick. looks neat!
looking forward see lambdas supported.
Hmm, trying this out in a downstream project, I think we didn't consider carefully enough what the shape of the dependency graph will be. If you just |
* feat(typescript): allow alternative transpilers Fixes #3133 * fixup! feat(typescript): allow alternative transpilers
* feat(typescript): allow alternative transpilers Fixes #3133 * fixup! feat(typescript): allow alternative transpilers
Fixes #3133