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

Corrected files included in package.json for bundler / no target (#837) #839

Merged
merged 5 commits into from
May 5, 2020

Conversation

lucashorward
Copy link
Contributor

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! πŸ˜„ ✨✨

Closes #837

wasm-pack build and wasm-pack build --target bundler generate a _bg.js file, but it is not added to the package.json. The file that is added, *.js will however reference the _bg.js, so when the package is distributed (both through pack or publish) it is not usable.

This change will fix that by passing the correct bool value to npm_data

As is the same for my other PR, there is some test failure that I locally also have on the master branch. Please let me know how to address this if possible

@lucashorward lucashorward changed the title Build files Corrected files included in package.json for bundler / no target (#837) May 4, 2020
Copy link
Contributor

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple small changes needed for this.

@@ -734,7 +734,7 @@ impl CrateData {
disable_dts: bool,
out_dir: &Path,
) -> NpmPackage {
let data = self.npm_data(scope, false, disable_dts, out_dir);
let data = self.npm_data(scope, true, disable_dts, out_dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be false. The to_commonjs method should also use false, only to_esmodules should be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then there would be no change in behavior, and the issue as described would not be fixed? Please let me know if there is a better way of fixing this issue!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the situation in the old code:

  • to_commonjs -> true
  • to_esmodules -> false
  • to_web -> false
  • to_nomodules -> false

This is the situation in your code:

  • to_commonjs -> true
  • to_esmodules -> true
  • to_web -> false
  • to_nomodules -> true

However it is supposed to be like this:

  • to_commonjs -> false
  • to_esmodules -> true
  • to_web -> false
  • to_nomodules -> false

It is only the bundler target (to_esmodules) which has the extra _bg.js file, the rest don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation - I have made the changes. However, I noticed in this and my previous pr that the test build_with_and_without_wasm_bindgen_debug keeps failing locally, and now I see that the same has happened on master. Originally I also had this issue on master locally so I thought it was a configuration issue on my side, but as it also fails in the CI I doubt that that is the case. Do you have any idea why this test fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I need to look into why that's failing, you can ignore that.

@Pauan Pauan merged commit 8eecad0 into rustwasm:master May 5, 2020
@Pauan
Copy link
Contributor

Pauan commented May 5, 2020

Thanks!

@Kurble
Copy link

Kurble commented Aug 18, 2020

Is a release scheduled anytime soon in order to fix this issue?

@Corey600
Copy link

Is a release scheduled anytime soon in order to fix this issue?

+1

@lucashorward
Copy link
Contributor Author

@ashleygwilliams

@igrep
Copy link

igrep commented Dec 29, 2020

Same here. This bug prevents us from running even a simple example (e.g. https://developer.mozilla.org/en-US/docs/WebAssembly/Rust_to_wasm). So this patch should be released soon.

@lucashorward
Copy link
Contributor Author

A workaround would be to reference this git repository directly in the cargo.toml:
wasm-pack = { git = "https://github.com/rustwasm/wasm-pack" }
But do note that that may include other unreleased changes that have been merged to master as well.

@igrep
Copy link

igrep commented Dec 30, 2020

Let me add an easier workaround: adding the _bg.js file to the generated package.json manually.

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.

wasm-pack build --target bundler not including *_bg.js file in package.json
6 participants