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

refactor(plugin_runner): split modules, introduce feature target #4012

Closed
wants to merge 8 commits into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Mar 14, 2022

Description:

This PR is internal refactoring to plugin_runner to support wasm host based target (@swc/wasm).

Notable changes are

  • plugin_runner now have embedded_runtime, native_runtime features. Embedded stands for existing plugin runner where we need to bring wasmer's runtime. Native means we'll use host runtime as-is, such as browser / node.js's v8 webassembly.
  • swc supports those new features via plugin_embedded_runtime / plugin_native_runtime features. Existing plugin is renamed into plugin_embedded_runtime.
  • plugin_runner extracts few logics into internal modules as well as branches to each features. Currently native_runtime is empty.
  • apply_js_plugin is now renamed to apply_transform_plugin to reflect what we do exactly. Further, we can possibly have other entrypoint like apply_xxx_plugin for css, or else.

@kwonoj kwonoj force-pushed the feat-plugin-runner-wasm-host branch 3 times, most recently from 598ce2f to b3a7f01 Compare March 14, 2022 22:04
@kwonoj
Copy link
Member Author

kwonoj commented Mar 14, 2022

lint failure comes from

  Checking swc_ecma_transforms_optimization v0.102.0 (/home/runner/work/swc/swc/crates/swc_ecma_transforms_optimization)
error: useless conversion to the same type: `simplify::dce::Data`
   --> crates/swc_ecma_transforms_optimization/src/simplify/dce/mod.rs:410:21
    |
410 |         self.data = data.into();
    |                     ^^^^^^^^^^^ help: consider removing `.into()`: `data`
    |
note: the lint level is defined here
   --> crates/swc_ecma_transforms_optimization/src/lib.rs:1:9

@kwonoj kwonoj force-pushed the feat-plugin-runner-wasm-host branch 2 times, most recently from 94a480b to 154a1a3 Compare March 14, 2022 22:45
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!

@@ -118,7 +120,7 @@ jobs:
- crate: swc
os: ubuntu-latest
check: |
cargo hack check --feature-powerset --no-dev-deps
cargo hack check --feature-powerset --no-dev-deps --exclude-features plugin_native_runtime
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to ran create-matrix.sh and nothing happens. What should be exactly updated to make sure script works?

Copy link
Member

Choose a reason for hiding this comment

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

You should copy output to the matrix to this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Output doesn't seem to correct to me. Looks like script requires jq / yq, but after installing both I'm getting

...
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6
- crate: testing_macros
  os: ubuntu-latest
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6
- crate: wasm
  os: ubuntu-latest
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6
parse error: Invalid numeric literal at line 1, column 6

Copy link
Member

Choose a reason for hiding this comment

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

Are you using windows?
Interesting as it specifies bash requirement... I don't think bash parse script differently based on the OS

Copy link
Member Author

@kwonoj kwonoj Mar 15, 2022

Choose a reason for hiding this comment

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

No, it was linux and managed to make it work - script requires to be executed cwd under scripts, does not allow ./scripts/create-matrix.sh from swc's root. It wasn't clear as there wasn't way to see error outputs.

@@ -27,8 +27,11 @@ concurrent = [
]
debug = ["swc_ecma_visit/debug"]
node = ["napi", "napi-derive"]
plugin = [
"swc_plugin_runner",
plugin_embedded_runtime = [
Copy link
Member

Choose a reason for hiding this comment

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

Why is this required?

I think user of the swc crate can depend on swc_plugin_runner/embedded_runtime or swc_plugin_runner/native_runtime instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably I'm making some mistakes if you felt bit odd. My thought was each package have deps to swc can select features without transitive deps feature selection, also swc itself uses own feature selection like https://github.com/swc-project/swc/pull/4012/files#diff-8987cc214ab0c7d63b833d31785ecb48d89d1bcc204d42508c26ecd72becc7b9R72 .

Copy link
Member

Choose a reason for hiding this comment

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

I think this is not required. I'll check using a fork

Copy link
Member

Choose a reason for hiding this comment

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

I think #4027 will work, except swc_cli on platforms not supported by cranelift. We should exclude them via --no-default-features

@kwonoj kwonoj force-pushed the feat-plugin-runner-wasm-host branch from 1afb4d0 to 2ffcea4 Compare March 15, 2022 06:36
@kwonoj kwonoj force-pushed the feat-plugin-runner-wasm-host branch 2 times, most recently from 14f790f to d8c1afb Compare March 15, 2022 06:49
@kwonoj
Copy link
Member Author

kwonoj commented Mar 15, 2022

Superseded by #4035.

@kwonoj kwonoj closed this Mar 15, 2022
@kwonoj kwonoj deleted the feat-plugin-runner-wasm-host branch March 15, 2022 21:24
@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants