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

[WIP] feat(build): allow specifying multiple targets #523

Closed
wants to merge 1 commit into from

Conversation

hobofan
Copy link

@hobofan hobofan commented Jan 24, 2019

Addresses #157 / #469

There are still a few things that should be done:

A few things that would be great to get your feedback on @ashleygwilliams :

  • I removed the EsModulesPackage, etc. and flattened that to the NpmPackage, as I there was quite some duplication there, and I didn't see a good way to achieve the feature without that. I hope that's okay 😬 .
  • Should I also tackle give option target=all which generates pkg supporting commonjs and esm #313 and refactor target to use an enum #332 in this PR, or better in a different PR?
  • I'm not sure if I should add a special case handling for the case of a single target, in which case the old file naming convention ({crate_name}.js) should be used instead of the new names ({crate_name}_{target}.js)? I guess that might be required for better compatibility?

Make sure these boxes are checked! 📦✅

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

@ashleygwilliams ashleygwilliams added the work in progress do not merge! label Jan 24, 2019
@ashleygwilliams
Copy link
Member

hey @hobofan ! thanks for this- it's late here for me but i'll take a look and leave some comments tomorrow morning. for a feature like this, one thing that's gonna be important is the expected final structure of the "isomorphic" package. could you share here in a comment the structure you anticipate?

thanks again!

@ashleygwilliams
Copy link
Member

@hobofan did you have a chance to read my previous comment? looks like we have some merge conflicts now... let me know where you are at with this PR and if there is anything i can do to help!

@ashleygwilliams
Copy link
Member

closing as out of date. @hobofan if you'd like to tackle this still, feel free to reopen this PR or make a new one!

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.

2 participants