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

Convert source to Buffer when getting asset's hash #1966

Merged
merged 3 commits into from
Mar 19, 2019

Conversation

3846masa
Copy link
Contributor

R: @jeffposnick @philipwalton

Fixes #1916

Description of what's changed/fixed.

asset.source() will return string or ArrayBuffer (Ref.) (or Buffer via some loaders), but Hash.update() shouldnot receive ArrayBuffer (Ref.).

Because of that, this PR enables to convert source to Buffer when getting asset's hash.

@coveralls
Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage remained the same at 82.7% when pulling 2a902da on 3846masa-tmp:fix-get-asset-hash into b56962a on GoogleChrome:master.

@jeffposnick
Copy link
Contributor

Thank you very much for your contribution!

I would like to add in a unit test to catch this scenario in the future, since we obviously did not have a test for this already. If you don't mind, I'll push the commit to your branch and include it in this PR.

To confirm, you were using https://github.com/ballercat/wasm-loader when you ran into this issue, correct?

@jeffposnick jeffposnick self-requested a review March 19, 2019 14:57
@jeffposnick
Copy link
Contributor

...or is it just the native webpack v4 WASM support that triggers this, like https://github.com/webpack/webpack/tree/master/examples/wasm-simple

@jeffposnick
Copy link
Contributor

Nevermind! I found your reproduction link to from the original issue, and I'll use that as the basis of the test: https://gist.github.com/3846masa/41a96d1fd4c192d973d2c5f772144d1f

Copy link
Contributor

@jeffposnick jeffposnick left a comment

Choose a reason for hiding this comment

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

I confirmed via the new test case that the test previously failed, and with the updated code, it now passes. Thanks again for the contribution!

@jeffposnick jeffposnick merged commit 7c5848e into GoogleChrome:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getAssetHash failed at generateMetadataForAssets
3 participants