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(swc/plugin): support context for the plugin transform #3568

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Feb 15, 2022

Description:

From discussion #3540 (comment).

Plugin may need additional data other than config to plugin itself, mostly state around swc/core. Some examples like

  • specific file name for current transform
  • maybe whole swcrc config, not only for the plugin itself

or possibly more. This PR attempts to resolve those by forwarding new param named context.

There are few possible approaches to enable this.

  1. expand plugin interface param signature
#[plugin_transform]
fn process(program: Program, plugin_config: String, filename: String, swcrc: String...);

Obviously, this is not preferrable as we'll continously expand signatures and each time we add new param it'll cause breaking changes.

  1. Typed struct param
struct PluginContext {
 filename: String,
 other: ...
}

#[plugin_transform]
fn process(program: Program, plugin_config: String, context: PluginContext);

This is most clear interface signature. However, have same pitfall as first. Each time we add new properties into struct, it'll be a breaking change since our serialization does not support add-only / versioned

  1. Context as JSON-string

Current approach. Safe to add new properties since it's string, plugin can try to pick up necessary properties for their use.

Though I picked 3rd, I'm currently bit debating 2 also - if we can add enough context properties in experimental phase, we can stabilize those to avoid frequent breaking changes. The part I'm not sure is how frequent we'll need to change this context information. Would like to hear thoughts, and update PR into second if needed.

BREAKING CHANGE:
Plugin will need to accept 3rd param in any case, it could be a string or a typed struct.

Related issue (if exists):

#3540 (comment).

@kwonoj
Copy link
Member Author

kwonoj commented Feb 15, 2022

Actually I'm slightly leaning toward to 2 at this moment. We'll need to figure out what properties we'll expose in that case.

kdy1
kdy1 previously approved these changes Feb 15, 2022
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thanks!
Can you rebase it?

@kwonoj kwonoj force-pushed the feat-plugin-context-support branch 2 times, most recently from 7b94c4f to b569a83 Compare February 15, 2022 03:57
@kwonoj
Copy link
Member Author

kwonoj commented Feb 15, 2022

Yup, was doing it already. 🚢

@kwonoj kwonoj force-pushed the feat-plugin-context-support branch from b569a83 to f56701b Compare February 15, 2022 06:15
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

I think we should rename variables.

@@ -424,6 +428,9 @@ impl Options {
40,
NodeModulesResolver::new(TargetEnv::Node, Default::default(), true),
);
let plugin_context = PluginContext {
source_file_name: source_file_name.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

This should use base instead.

Copy link
Member

Choose a reason for hiding this comment

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

source_file_name is used to control sourcemap, and base is the filename.
It's named base because the parameter is introduced because of paths support, but I think it's a bad name.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I was confused by name actually.

@kwonoj kwonoj force-pushed the feat-plugin-context-support branch from f56701b to add326e Compare February 15, 2022 07:07
Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

LGTM.

@kdy1 kdy1 enabled auto-merge (squash) February 15, 2022 07:12
@kdy1 kdy1 added this to the v2.0.0-alpha.1 milestone Feb 15, 2022
@kdy1 kdy1 merged commit a96217f into swc-project:main Feb 15, 2022
@kwonoj kwonoj deleted the feat-plugin-context-support branch February 15, 2022 08:04
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants