-
Notifications
You must be signed in to change notification settings - Fork 418
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
Conversation
There was a problem hiding this 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.
src/manifest/mod.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks! |
Is a release scheduled anytime soon in order to fix this issue? |
+1 |
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. |
A workaround would be to reference this git repository directly in the cargo.toml: |
Let me add an easier workaround: adding the |
Make sure these boxes are checked! π¦β
rustfmt
installedcargo fmt
on the code base before submittingβ¨β¨ π Thanks so much for contributing to wasm-pack! π β¨β¨
Closes #837
wasm-pack build
andwasm-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 throughpack
orpublish
) 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