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 a --target option target different environment (nodejs, browser) #125

Closed
wants to merge 3 commits into from

Conversation

djfarly
Copy link
Contributor

@djfarly djfarly commented May 19, 2018

Implements #75

I had some time in the train – I hope it's okay.
Some lines of documentation and a few line breaks for readability included.

Thoughts?

Make sure these boxes are checked! πŸ“¦βœ…

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ πŸ˜„ Thanks so much for contributing to wasm-pack! πŸ˜„ ✨✨

@djfarly djfarly mentioned this pull request May 19, 2018
@djfarly
Copy link
Contributor Author

djfarly commented May 19, 2018

i'll look at rustfmt in a minute

@mgattozzi
Copy link
Contributor

This LGTM, I think we just need you to format it with the latest rustfmt :)

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.

hi! so thanks for starting on this. one of the reasons i had assigned this to myself tho is that i have a very specific design for it. wasm-bindgen has several non-compatible flags that affect the js output (https://github.com/rustwasm/wasm-bindgen#cli-reference). i dont personally care for this design because it allows you to pass several flags even tho they do not work together at all. i would like the design to be something like --module or --output that takes a single argument, one of the flags listed in the link.

@djfarly
Copy link
Contributor Author

djfarly commented May 24, 2018

Mea culpa β€” sorry for the steal.
I'm fine to drop this if you prefer to tackle it yourself, no hard feelings. πŸ™ƒ

I see where you're going and like the idea.

Some bikeshedding:
I think --module is confusing as a name. --output describes it quite well. Personally I'd go for --target in the style of webpack.

What would be the valid options currently? I find the bindgen docs a bit confusing.

I guess currently only those?

  • --target nodejs
  • --target browser

/edit rollup uses format which also seems fine to me ;)

@djfarly
Copy link
Contributor Author

djfarly commented May 25, 2018

Does the update meet your expectations a bit more closely?

As per TeXitoi/structopt#104, TeXitoi/structopt#110 I don't see a simple way to allow a list of possible values (an enum?).

@djfarly
Copy link
Contributor Author

djfarly commented May 25, 2018

I'm not quite sure why travis fails - I tried updating rustfmt…

The log says:

0.10s$ cargo fmt -- --write-mode diff
Unrecognized option: 'write-mode'

πŸ€”

@djfarly djfarly mentioned this pull request May 26, 2018
3 tasks
@djfarly djfarly changed the title Nodejs flag Add a --target option target different environment (nodejs, browser) May 26, 2018
@ashleygwilliams
Copy link
Member

@djfarly - no worries on "stealing", i meant it more as an apology for filing a very vague feature issue πŸ˜…

--target sounds great to me, it's both analogous to rust and webpack, which is perfect IMO.

FYI the travis failure was a breaking change in rustfmt that we(lol, actually YOU! https://github.com/ashleygwilliams/wasm-pack/pull/128) fixed.

as for what should be supported- you can see the flags here: https://github.com/rustwasm/wasm-bindgen#cli-reference. i agree that the docs are a bit unwiedly to navigate- which is partially why wrapping wasm-bindgen up into wasm-pack will hopefully make it an easier user experience!

i'm gonna take a slightly closer look at this- but i think we should probably be able to merge this today, or at least in the next few days! thank you so much for your efforts on this!!

@ashleygwilliams
Copy link
Member

slurped this up, rebased it, added docs in https://github.com/ashleygwilliams/wasm-pack/pull/132.

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.

None yet

3 participants