-
Notifications
You must be signed in to change notification settings - Fork 409
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
Fix sideEffects value in generated package.json should be boolean #649
Conversation
CI failed due to the reason not related to changes of this PR. https://travis-ci.com/rustwasm/wasm-pack/jobs/201309430
I guess this is because multiple test cases use the same directory name for test. As you know, Rust runs tests parallelly. So when using the same directory for multiple tests, the directory may be (or may not be) used by other test cases. It would cause some flakiness on testing. I'll try to resolve it and check if my guess is correct or not |
Travis CI job is now back to normal. Failure on AppVeyor also seems to occur on master branch so it is not related to changes on this PR. |
@@ -28,17 +28,17 @@ fn it_should_build_crates_in_a_workspace() { | |||
"Cargo.toml", | |||
r#" | |||
[workspace] | |||
members = ["blah"] |
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.
i'm not sure i understand this change- can you explain? otherwise this looks good!
gonna snag this PR and try to work through the test failures. thanks for noticing this! it's a BIG thing to catch! |
cannot replicate this locally. can you perchance @steveklabnik ? this is failing on the workspaces test: https://ci.appveyor.com/project/ashleygwilliams/wasm-pack-071k0/builds/24651407#L394 |
I get eight test failures on both master and with this PR, so I don't think this PR is causing it directly. |
i'm gonna remove the commit on this that attempted to address the failures and then merge since i dont think it is related.. we'll see what CI says |
closing in favor of #680 |
#649 without the test update commit
Make sure these boxes are checked! π¦β
rustfmt
installedcargo fmt
on the code base before submittingβ¨β¨ π Thanks so much for contributing to wasm-pack! π β¨β¨
I'm not heavy user of webpack but
sideEffects
field ofpackage.json
seems boolean or array value.https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free
But current wasm-pack sets
"false"
string value to the field. This patch fixes the point.