-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Reflect child bundle contents in own bundle hash #3414
Conversation
let hash = crypto.createHash('md5'); | ||
// TODO: sort?? | ||
this.traverseAssets(bundle, asset => { | ||
hash.update(asset.outputHash); | ||
}); | ||
|
||
let hashHex = hash.digest('hex'); | ||
this._bundleContentHashes.set(bundle.id, hashHex); |
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.
Since computing the hash for a bundle how involves computing content hashes for its entire bundle subgraph (in cases where there's only one entry, this is every bundle), cache them.
@mischnic, @magcius, @Drapegnik: can you confirm this fixes the issues you found? |
c56dbd7
to
988759f
Compare
Yes! |
@wbinnssmith, works for me! |
Hmm won’t this mean that any time a child bundle is invalidated, all of the ancestors will also be invalidated? Kinda negates the point of content hashing for cacheability no? |
Aren't they rather for cache busting? |
The other option here would be to provide a system for "this asset has a cached item reference as a dependency". That would allow for CSS files that reference image assets, etc. to get invalidated when the image asset changes, without invalidating the entire bundle at once. |
Any bundle could end up referencing any of its child bundles via its name/url, and recursively so.
It currently does not, as the fact that entry bundles do not contain content digests is a detail of the This is a conservative approach for sure, and could possibly miss situations where we reference another bundle's name that factors in something other than content. I'm open to suggestions. |
17d218b
to
31f3397
Compare
31f3397
to
035aa46
Compare
@@ -42,7 +42,6 @@ export async function ncp(source: FilePath, destination: FilePath) { | |||
.on('error', reject); | |||
}); | |||
} else if (stats.isDirectory()) { | |||
outputFS.mkdirp(destPath); |
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 wasn't awaited, and it also happens anyway in the next recursive call to ncp
.
Resolves #3393 and Resolves #3409
Since we include a bundle's hash in filenames, anything that could possibly alter the content must be factored into what a bundle's hash is, including that a bundle can reference another bundle with a changing digest.
This extracts the current hashing strategy into
_getContentHash
, which returns a hash of the bundles own content. IngetHash
, recursively retrieve child bundle's content hashes and reflect them in the current bundle's hash. UsingtraverseBundles
, which now has the option to start at a given bundle, will prevent cyclic bundle dependencies from being considered.Test Plan:
contentHashing
integration test.--no-cache
from the react-hmr example and demonstrate JS filename not updated in script tag after hash change #3393 before applying this patch. Apply the patch and verify JS filename not updated in script tag after hash change #3393 is no longer reproducible.