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

Fix non-updating asset hashes #1861

Merged
merged 5 commits into from Aug 24, 2018
Merged

Fix non-updating asset hashes #1861

merged 5 commits into from Aug 24, 2018

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Aug 7, 2018

Include asset content in asset hash, as generated not always outputs the entire ast and makes it unreliable. (Only content would probably also be unreliable)

Should prevent some cases where assets don't get updated either from cache or HMR

Closes #1789

@DeMoorJasper DeMoorJasper changed the title add content hash to asset hash Fix non-updating asset hashes Aug 7, 2018
src/Asset.js Outdated
let contentHash = '';
try {
contentHash = md5(this.contents);
} catch (err) {
Copy link
Member Author

Choose a reason for hiding this comment

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

In case this.contents is not a string or undefined

@devongovett
Copy link
Member

devongovett commented Aug 22, 2018

Maybe the hash should be based on the contents after post processing instead of generate also? That seems to be related to this issue.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Aug 22, 2018

So moving hash generation to the Pipeline after postProcess?

Or just merging the asset hash with postProcessed output hash?

@devongovett
Copy link
Member

Yeah I guess? I'd say just hash the original content but sometimes we don't actually read it (so you have the try..catch). Try just hashing the post-processed output instead of generate and see if that fixes the bug.

@DeMoorJasper
Copy link
Member Author

Changed it, should still fix the issue. As it's pretty much doing the same. Just on output instead of input. (which is probably more reliable for virtual assets like globs)

src/Asset.js Outdated
@@ -214,7 +213,7 @@ class Asset {
}

generateHash() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this method needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

RawAsset uses it as it has no generated content

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep the method then and just call it after post-processing is done?

@devongovett
Copy link
Member

Looks like the tests are failing because the yarn lockfile is pointing at adobe corp artifactory. Can you fix?

@DeMoorJasper
Copy link
Member Author

@devongovett I think it ended up in the master branch by accident, can't seem to find it in this branch.

Appears to be the flow deps, I'll open a seperate PR to fix that

@devongovett devongovett merged commit 8a743f3 into master Aug 24, 2018
@devongovett devongovett deleted the fix/asset-hashe branch August 24, 2018 19:20
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
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.

None yet

3 participants