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

feat(build): add first-class support for binary crates #736

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

boringcactus
Copy link

Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed
$ rustup component add rustfmt
  • You ran cargo fmt 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! 😄 ✨✨


fixes #734

Now that rustwasm/wasm-bindgen#1630 has been implemented, I think it's good to have the same support in wasm-pack.

This implementation currently doesn't add bin entries in package.json, partially because the docs on that field say that targets must be prefixed with #!/usr/bin/env node and I'm not sure how or where to ensure that all and only binaries get that prefix. I don't think that feature is vital, but if other people think it's a must-have then I can take a look at adding it.

bors bot added a commit to grovesNL/glow that referenced this pull request Nov 22, 2019
61: Instructions for the stdweb backend in the hello example. r=grovesNL a=theypsilon

Previously there weren't instructions to run the 'hello' example on stdweb, so I'm adding them.

But I also noticed that the README.md is not quite right yet. The web-sys backend instructions seem obsolete, but I can't find and easy way to make it work with wasm-pack because 'hello' is a binary crate.

Here is a pull request to make wasm-pack compatible with binary crates: rustwasm/wasm-pack#736

Co-authored-by: José manuel Barroso Galindo <theypsilon@gmail.com>
@boringcactus
Copy link
Author

currently the way to get examples built is to throw --bins --examples straight to cargo build. should we accept a --example foo argument instead? that's probably cleaner but also a little bit more work on the implementation side

@boringcactus
Copy link
Author

--example foo now works.

@boringcactus
Copy link
Author

boringcactus commented Jan 11, 2020

issue with the original draft of this PR: if a binary is defined but required features aren't available, it doesn't get built, but this code still tries to run wasm-bindgen against it, causing problems. i fixed it, though.

this handles, e.g., binaries missing required features
@ashleygwilliams ashleygwilliams modified the milestones: 0.9.0, 0.10.0 Jan 31, 2020
Copy link

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Honestly I can't find anything to complain about here, I also tried it locally, seems to work perfectly.
As mentioned in my review, I think the PR can be kept simpler if we don't try to handle compiling multiple packages at once here and leave that to #732.

Is there anything else that can be done to move this forward?
@boringcactus could I give you a hand rebasing this?

src/bindgen.rs Show resolved Hide resolved
src/manifest/mod.rs Show resolved Hide resolved
@alvinhochun
Copy link

Other than a rebase (which #816 tried to do), is there anything else that is blocking the merge of this PR?

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

Successfully merging this pull request may close these issues.

support building binary crates
4 participants