aspect-build / rules_swc

Bazel rules for the swc toolchain https://swc.rs/
https://docs.aspect.build/rules/aspect_rules_swc
Apache License 2.0
44 stars 24 forks source link

Add support for plugins #149

Closed titanous closed 1 year ago

titanous commented 1 year ago

This PR implements all of the wiring necessary to use SWC plugins.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

realtimetodie commented 1 year ago

This really nice. Thank you. My concern is the substitution of the user's configuration. It just feels wrong and complicates things.

If you are already patching the cli, I would go a step further and introduce a new --plugin multi-option. This can be done during the initialization phase

https://github.com/swc-project/swc/blob/47cc6446d4e2cd86846e8de5b67d279ae463dcc4/bindings/swc_cli/src/commands/compile.rs#L268

titanous commented 1 year ago

I agree that adding a CLI flag makes sense. It also may be better for upstream because they can punt on relative paths in the config. I came up with this design when I thought that it could work without modifying SWC at all 😅

realtimetodie commented 1 year ago

I think by adding a new option you can also resolve the issue with the base path in https://github.com/swc-project/swc/pull/6800.

Providing a path using the --plugin option should work in the same way as providing a path using a --config option.

titanous commented 1 year ago

Updated to use CLI arguments added in https://github.com/swc-project/swc/pull/6811

Much better, thanks for the suggestion, @realtimetodie!

alexeagle commented 1 year ago

Hey @titanous thanks for the contribution, sorry I didn't get to it earlier. I rebased over #146 which mirrored the latest swc version, and dropped your commit that switched to your custom fork.

I think this would be nice to get into the 1.0 :)

titanous commented 1 year ago

https://github.com/swc-project/swc/pull/6835 has now been merged, so as soon as a new version of SWC is released this PR should be ready to go.

alexeagle commented 1 year ago

upstream cut the release, so I think this is unblocked now @titanous :)

titanous commented 1 year ago

Indeed, the tests are now passing locally and this is ready for review.

alexeagle commented 1 year ago

The added wasm file is 2MB which will severely bloat our download for users of the ruleset. Can we fetch it with http_file from somewhere instead?

realtimetodie commented 1 year ago

Yes, we should not add binary files, wasm or not.

Luckily, there is rules_rust which can be used to ceate a plugin with the wasm bindgen rules.

https://bazelbuild.github.io/rules_rust/rust_wasm_bindgen.html

Rust wasm bindgen is a Rust library and CLI tool that facilitates high-level interactions between wasm modules and JavaScript. This would also be a great example for users.

titanous commented 1 year ago

PTAL

titanous commented 1 year ago

PTAL