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 --cargo-build and --wasm-bindgen for passing through options #805

Closed
wants to merge 2 commits into from

Conversation

silvanshade
Copy link

This PR adds two new flags: --cargo-build and --wasm-bindgen.

These flags take comma-separated arguments (e.g., --cargo-build=--foo,--bar,--baz) and pass them on to the respective tool.

See #799 for more background discussion on what motivated these flags.

In short, we need a way to pass --omit-imports on to wasm-bindgen and currently there is no way to do that unless:

  1. we also add an --omit-imports flag to wasm-pack, or
  2. we provide a more general mechanism to pass on arguments

Option (2) is more general so I've opted for that approach in this PR.

Since --cargo-build=--foo,--bar,--baz does the same thing as -- --foo --bar --baz, I've added a deprecation notice for the latter in wasm-pack build --help. A warning will also be emitted during the build process if any arguments are actually passed via --.

If both --cargo-build and -- <extra_options> are specified, the latter is ignored (with a warning).

If you would prefer not to have the deprecation warnings, I can remove those. Also happy to change the names of the flags if anyone has better ideas about that.

@Pauan
Copy link
Contributor

Pauan commented May 4, 2020

Hey, sorry for the delay on this. I'm going to start taking over the responsibilities of wasm-pack, and I have some time to do some reviews, if you're willing to resubmit this PR.

@silvanshade
Copy link
Author

Sure, no worries, happy to re-submit.

@silvanshade
Copy link
Author

silvanshade commented May 5, 2020

I'm not sure why some of the tests are failing. On my system (toolchain stable-x86_64-pc-windows-msvc) the only test that is failing is build::build_with_and_without_wasm_bindgen_debug. But I'm also seeing that one fail on master so I think it's unrelated to the changes here.

Please let me know if you need me to change something.

@Pauan
Copy link
Contributor

Pauan commented May 13, 2020

@darinmorrison I fixed the CI errors. This generally looks good, but I'd really prefer if it used spaces for the separator, like --cargo-build "foo bar qux". Maybe it can use value_delimiter(" ") to accomplish that?

@silvanshade
Copy link
Author

@Pauan alright. I updated it to use a space for the delimiter.

@Pauan
Copy link
Contributor

Pauan commented May 19, 2020

@darinmorrison Thanks! Could you add a test as well (under tests/all)?

@silvanshade
Copy link
Author

@Pauan okay, I've added some tests.

@ashleygwilliams
Copy link
Member

hey folks- are we sure this is the design we think is best? i'm a bit worried about the deprecation. i think this feature would be much simpler and fitting with the design of wasm-pack if it was handled in the config like we handle wasm-opt options

wasm-pack's CLI interface is meant to be ergonomic, this feels a lot like something that should be in config being a command line interface.

Copy link
Member

@ashleygwilliams ashleygwilliams 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 that we should discuss if this is the interface design that makes the most sense- these flags are long and feel much more like config to me than something we should be putting in the CLI

@Pauan
Copy link
Contributor

Pauan commented May 21, 2020

@ashleygwilliams I'm okay with it being a config option, though that means you wouldn't be able to vary it if you're building multiple times.

On the other hand, a major benefit of a config option is that we can use arrays for the arguments:

[package.metadata.wasm-pack]
wasm-bindgen = ["foo", "bar"]

@chinedufn
Copy link
Contributor

A potential consideration:

Config + ability to use an environment variable override might be nice for things like this going forwards

As far as I can tell you can only use config for wasm-opt config which makes it tricky to sometimes do X but other times do Y in my personal build scripts

Config + Env Var is the design that cargo uses

WASM_PACK_WASM_BINDGEN_FLAGS=“foo bar” perhaps.

@Pauan
Copy link
Contributor

Pauan commented May 25, 2020

After thinking about this some more, I think we do need some way to customize it per invocation, because the various plugins (like the Webpack and Rollup plugins) might want to use --cargo-build and --wasm-bindgen, and they cannot modify the Cargo.toml.

So we either need to use arguments (like this PR), or env vars (like @chinedufn mentioned). I think passing arguments to cargo build and wasm-bindgen are common enough that it justifies using arguments, but I don't have a strong opinion about it.

(Another more radical approach would be to allow for plugins to pass in the Cargo.toml file to wasm-pack, that would allow them to fully customize all of the Cargo.toml options)

@ashleygwilliams
Copy link
Member

i think i like the env var idea- i think having env vars for anything that could be in a config makes sense. if/when for adding something like this to the command line interface, i'd want to come up with some named defaults- i think if we release this with config and env vars and keep track of how people are using it we'll have more info to think about what an interface via the CLI could look like. i'm not sure we have that info now and don't want to hold us to a deprecation or an interface that we aren't yet sure of!

tl;dr - let's do config and env vars

@ashleygwilliams
Copy link
Member

oh also- @Pauan i think a plugin system for customized builds is a very interesting idea. perhaps we should start a thread to brainstorm on that. as more and more people use this for more and more things, we're not going to be able to fulfill all the needs in a single coherent interface, so giving people an interface to build their own seems like the right move! (though you are right it will be a pretty radical move- i think potentially a good one tho!)

@silvanshade
Copy link
Author

silvanshade commented Jun 4, 2020

I don't have much to add except to say that I'm willing to take a look at switching this over to using env vars and potentially config options as well.

I'm a little worried about scope creep with tackling the bigger issue of generalizing support of all config options to env vars though. I'm not against the idea but I think maybe it would be better to split that out into a new issue and/or PR, especially since there will probably need to be more discussion.

In any case, it may be a while before I have a chance to rework this since I'm trying to get a big release done for another project at the moment. However, if anyone wants to tackle this before I get a chance to do so, that's okay with me.

@Pauan
Copy link
Contributor

Pauan commented Jun 4, 2020

@darinmorrison Yes, I agree that we'll figure out the env/config stuff in another PR.

For this PR I think it's enough to just add in support for Cargo.toml config (we can handle the rest later). No rush.

@silvanshade
Copy link
Author

I'm just going to close this out. The requested changes will require a completely different implementation and I'm not sure when or even if that will happen at this point.

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.

5 participants