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(typescript): allow alternative transpilers #3134

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Conversation

alexeagle
Copy link
Collaborator

Fixes #3133

Copy link
Collaborator

@thesayyn thesayyn left a 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
Copy link
Collaborator

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": "...",
}

Copy link
Collaborator

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)

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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

@alexeagle alexeagle force-pushed the ts_transpiler branch 2 times, most recently from d4e6099 to c9cfc1a Compare December 22, 2021 00:15
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)`
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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)

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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)`
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@alexeagle alexeagle force-pushed the ts_transpiler branch 2 times, most recently from 346dab4 to a76ab93 Compare January 3, 2022 22:47
Copy link
Collaborator

@thesayyn thesayyn left a 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.

@alexeagle alexeagle merged commit dc0dc8f into stable Jan 4, 2022
@alexeagle
Copy link
Collaborator Author

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 dep on a ts_project with this attribute set, you get .d.ts files but no way to collect up all the .js outputs. @dgp1130 since Angular has been thinking about type-check independent from the dev workflow maybe you have some minutes to chat about it with me?

alexeagle added a commit that referenced this pull request Jan 7, 2022
* feat(typescript): allow alternative transpilers

Fixes #3133

* fixup! feat(typescript): allow alternative transpilers
alexeagle added a commit that referenced this pull request Jan 8, 2022
* feat(typescript): allow alternative transpilers

Fixes #3133

* fixup! feat(typescript): allow alternative transpilers
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.

ts_project should allow alternative transpilers
3 participants