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

Add --omit-imports flag #799

Closed
wants to merge 1 commit into from

Conversation

silvanshade
Copy link

This PR adds an --omit-imports flag corresponding to the wasm-bindgen-cli flag added upstream in rustwasm/wasm-bindgen#1958.

This flag is useful for some scenarios where imports must be controlled through separate preloads scripts where items are brought into the application scope by modifying global or window directly, instead of through the usual import or require mechanisms.

Examples of using this flag can be found in the omit-imports branch of electron-sys. See the activity-monitor example (with package.json and preload.js) and hash example with (with package.json and preload.js).

@Pauan
Copy link
Contributor

Pauan commented Feb 21, 2020

Hmmm, I'm not sure if this is a good idea, we don't add all of the wasm-bindgen flags, especially the more obscure ones.

It's always possible to use the wasm-bindgen flags by using --, like this:

wasm-pack build -- --omit-imports

@silvanshade
Copy link
Author

@Pauan are you sure that actually works? When I try modifying the npm script in the first example from

"pack-app-renderer": "cd crates/app-renderer && wasm-pack build --omit-imports --target web"

to

"pack-app-renderer": "cd crates/app-renderer && wasm-pack build --target web -- --omit-imports",

it does not work, but errors with the following:

> cd crates/app-renderer && wasm-pack build --target web -- --omit-imports

[INFO]: Checking for the Wasm target...
[INFO]: Compiling to Wasm...
error: Found argument '--omit-imports' which wasn't expected, or isn't valid in this context

From wasm-pack build --help, it sounds like -- <extra_options> get passed to cargo build but we need them to be passed to wasm-bindgen.

Am I missing something?

@Pauan
Copy link
Contributor

Pauan commented Feb 22, 2020

@darinmorrison You're right, I guess I got confused. We really should have a way to send arbitrary arguments to wasm-bindgen.

The only reason I see to send arguments to cargo build is for --features. So I think this would be better:

  • Add in --features, --all-features, and --no-default-features to wasm-pack.

  • Change -- to pass the extra arguments to wasm-bindgen rather than cargo.

However, this is a breaking change. @ashleygwilliams what do you think?

@silvanshade
Copy link
Author

Thanks for the clarification. A couple thoughts:

First, I agree exposing --omit-imports isn't great, since it is a more obscure flag. I was hesitant to add this but I didn't see an alternative.

Second, maybe it is worth considering another approach that would accomplish something similar to what you are describing but without being a breaking change.

We could add two new flags: --cargo-options and --bindgen-options. These would take something like a quoted argument string or comma separated arguments and pass them on to the respective tool. Then, -- <extra_options> could be kept as-is (for now anyway), perhaps with a deprecation warning.

I don't have a strong opinion about doing it this way versus what you proposed. I could imagine it might still be useful to be able to pass arguments to cargo build for some things, like experimenting with new features or debugging, but I don't have any real evidence for that.

But doing it this way with explicit *-options flags might be more flexible and would also generalize in case new tools are added to the pipeline in the future.

@Pauan
Copy link
Contributor

Pauan commented Feb 24, 2020

We could add two new flags: --cargo-options and --bindgen-options.

I think that's reasonable. Though --wasm-bindgen-options is a bit of a mouthful. But it's only used for obscure options, so I think it's okay. Maybe it could be shortened to --wasm-bindgen?

@silvanshade
Copy link
Author

Closing in favor of #805.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants