Skip to content

Conversation

@titanous
Copy link
Contributor

@titanous titanous commented Jan 12, 2023

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

  • Add a swc_plugin macro/rule pair that creates a target with the necessary providers:
    • a DefaultInfo pointing at a wasm file or an npm package containing a wasm file as the main entrypoint in the package.json
    • a new SwcPluginConfigInfo holding the plugin configuration
  • Add a plugins attr to swc_compile and swc, taking a list of plugin targets, along with helpers to inject the plugin config.

@CLAassistant
Copy link

CLAassistant commented Jan 12, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Ooh, that's interesting. Thanks for working on this! Let's see what they decide upstream about the resolution pathing.

@realtimetodie
Copy link
Contributor

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
Copy link
Contributor Author

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

I think by adding a new option you can also resolve the issue with the base path in swc-project/swc#6800.

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

@titanous
Copy link
Contributor Author

Updated to use CLI arguments added in swc-project/swc#6811

Much better, thanks for the suggestion, @realtimetodie!

@alexeagle alexeagle added this to the 1.0 milestone Feb 5, 2023
@alexeagle
Copy link
Member

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
Copy link
Contributor Author

titanous commented Feb 5, 2023

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

@gregmagolan gregmagolan added enhancement New feature or request blocked Blocked by another issue labels Feb 6, 2023
@alexeagle
Copy link
Member

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

@titanous titanous marked this pull request as ready for review February 8, 2023 16:27
@titanous
Copy link
Contributor Author

titanous commented Feb 8, 2023

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

@titanous titanous force-pushed the plugins branch 2 times, most recently from 7b1f945 to b132161 Compare February 8, 2023 16:42
@alexeagle
Copy link
Member

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

realtimetodie commented Feb 8, 2023

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 titanous force-pushed the plugins branch 2 times, most recently from c8735b8 to fb14b53 Compare February 8, 2023 22:28
@titanous
Copy link
Contributor Author

titanous commented Feb 8, 2023

PTAL

@titanous
Copy link
Contributor Author

titanous commented Feb 8, 2023

PTAL

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

plugin_cache = []
plugin_args = []
if ctx.attr.plugins:
plugin_cache = [ctx.actions.declare_directory("{}_plugin_cache".format(ctx.label.name))]
Copy link
Member

Choose a reason for hiding this comment

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

very creative :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ha, I was pretty proud of myself when I came up with that one

@alexeagle alexeagle removed the blocked Blocked by another issue label Feb 8, 2023
@alexeagle alexeagle merged commit 9897017 into aspect-build:main Feb 8, 2023
@titanous titanous deleted the plugins branch February 8, 2023 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants