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

chore(plugin): expose optional features for wasm plugin test #4351

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aircloud
Copy link
Contributor

Description:

At present, the default features of wasmer are not exposed by default, so if third-party plugin developers want to use the swc to write some test code, it will be difficult.

For example, if there is no default feature of wasmer, developers using related capabilities will report errors like the following:

error: At least the `host-fs` or the `mem-fs` feature must be enabled. Please, pick one.
 --> /Users/xxx/.cargo/registry/src/mirrors.tuna.tsinghua.edu.cn-df7c3c540f42cdbd/wasmer-vfs-2.2.1/src/lib.rs:9:1
  |
9 | compile_error!("At least the `host-fs` or the `mem-fs` feature must be enabled. Please, pick one.");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Moreover, I haven't found a way to directly specify the dependency's dependency features in rust, so I think adding some optional features to swc may be a solution. This will not affect normal use.

Of course, if there are other ways to accomplish this ability, it is also OK

Here's an my test code example based on other developers' versions in the community(
The test now fails to compile): https://github.com/aircloud/swc-plugin-console-prefix/blob/feat/swc_2022_04/src/lib.rs

BREAKING CHANGE:

None

Related issue (if exists):

None

@kdy1 kdy1 requested a review from kwonoj April 17, 2022 14:22
@@ -29,6 +29,9 @@ concurrent = [
debug = ["swc_ecma_visit/debug"]
node = ["napi", "napi-derive"]
plugin = ["swc_plugin_runner", "swc_plugin_proxy/plugin-rt"]
swc_plugin_runner_default = ["swc_plugin_runner/default"]
Copy link
Member

Choose a reason for hiding this comment

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

I'm against this change.
This will make CI much slower.
Users can depend on wasmer by themselves

Copy link
Contributor Author

@aircloud aircloud Apr 18, 2022

Choose a reason for hiding this comment

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

Users can depend on wasmer by themselves won't solve the problem well.

But I agree with the latter point of view. I can turn off this PR first and wait for your solution~

Copy link
Member

@kwonoj kwonoj left a comment

Choose a reason for hiding this comment

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

Mainly, I'm a bit hesitant to expose explicitly wasmer_* as features to the downstream who want to use. Runtime we use should be implementation detail we can change anytime.

@kdy1
Copy link
Member

kdy1 commented Apr 17, 2022

@kwonoj I think you are right. So we should expose a feature, right?
Otherwise user have to depend on wasmer

@kwonoj
Copy link
Member

kwonoj commented Apr 17, 2022

yes, I think it makes sense swc* exposes a feature to let user controls. But name should not include anything 'wasmer'.

@aircloud
Copy link
Contributor Author

So I am waiting for you to add this feature, should this PR be closed?

@kdy1 kdy1 marked this pull request as draft April 18, 2022 04:09
@kwonoj
Copy link
Member

kwonoj commented Apr 18, 2022

I think you can update PR to rename features, also to not to try to include some features by default.

@aircloud
Copy link
Contributor Author

aircloud commented Apr 20, 2022

I think you can update PR to rename features, also to not to try to include some features by default.

I'll think about it and update it later

@aircloud
Copy link
Contributor Author

aircloud commented Apr 20, 2022

I thought again about the related issue

I found that in addition to the problem I reported above, after enabling the plugin feature, there is a simple problem that will cause the simplest import to fail to compile.

// in Cargo.toml:
[dependencies]
swc = { version = "0.161.1",  features = ["plugin"] } 

// in main.rs:
use swc;

// error:
error: At least the `host-fs` or the `mem-fs` feature must be enabled. Please, pick one.
 --> /Users/aircloud/.cargo/registry/src/mirrors.tuna.tsinghua.edu.cn-df7c3c540f42cdbd/wasmer-vfs-2.2.1/src/lib.rs:9:1
  |
9 | compile_error!("At least the `host-fs` or the `mem-fs` feature must be enabled. Please, pick one.");
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

   Compiling region v3.0.0

Combined with the problems here, I think it is necessary to expose an additional feature to use swc with plugin function for rust developers.

So I re-integrated and called this feature plugin-dev temporarily.

For rust developers, just use plugin-dev.

Other names are OK, but there should be such a feature right?

how about this? @kwonoj

@kdy1
Copy link
Member

kdy1 commented Apr 20, 2022

Plugin authors are not supposed to depend on swc

@aircloud
Copy link
Contributor Author

Plugin authors are not supposed to depend on swc

emm, Is this sample code(just for test) not recommended? @kdy1

https://github.com/aircloud/swc-plugin-console-prefix/blob/feat/swc_2022_04/examples/usage.rs

@kdy1
Copy link
Member

kdy1 commented Apr 20, 2022

Yes, it's not recommended

@kwonoj
Copy link
Member

kwonoj commented Apr 20, 2022

As already explained, it is discouraged to have swc, or anything else as a dependency to the plugin. The only deps plugin may rely on is swc_plugin. (There are some exceptions like swc_ecma_quote due to proc_macro.)

@aircloud
Copy link
Contributor Author

aircloud commented Apr 21, 2022

As already explained, it is discouraged to have swc, or anything else as a dependency to the plugin. The only deps plugin may rely on is swc_plugin. (There are some exceptions like swc_ecma_quote due to proc_macro.)

I know this

But I do sometimes want to write a little rust test in the plugin repository, so I hope to introduce swc in [dev-dependencies]

[dependencies]
swc_plugin = "0.47.0"

[dev-dependencies]
swc = { version="xx", features = ["plugin", "plugin-dev"] }
swc_common = {  version="xx",, features = ["tty-emitter"] }

But you don't seem to recommend this approach, I'm very confused now.

So how to write rust test code for the plugin (Mock a real calling environment instead of simple unit tests)? @kwonoj

@kwonoj
Copy link
Member

kwonoj commented Apr 21, 2022

Most of cases test would be performed with swc_ecma_transforms_testing. Yes, it is a unit test.

It is not impossible to run whole via swc, but we do not recommend it as standard practice, and you may try it on your own.

@aircloud
Copy link
Contributor Author

Thank you, I understand what you mean

I don't seem to have any more ideas for this PR (although If no feature is to be added, It is hard to use swc to test the whole case in rust, like i said before. Maybe I just have to give up using it like this)

@kdy1 kdy1 added this to the Planned milestone Jun 25, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.2.210 Jul 5, 2022
@kdy1 kdy1 removed this from the Planned milestone Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants