Incorporate child bundle ids in BundleGraph.getHash(bundle) #4189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
padmaia
requested review from
wbinnssmith,
devongovett,
mischnic and
DeMoorJasper
Benchmark Resultspackages/benchmarks/kitchen-sink ✅
Timings
Cold Bundles
Cached Bundles
packages/benchmarks/react-hn ✅
Timings
Cold Bundles
Cached Bundles
packages/benchmarks/ak-editor ✅
Timings
Cold Bundles
Cached Bundles
|
wbinnssmith
approved these changes
Feb 20, 2020
devongovett
approved these changes
Feb 21, 2020
wbinnssmith
approved these changes
Feb 21, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
↪️ Pull Request
Before the addition of bundle content hash references (#4114),
BundleGraph.getHash(bundle)incorporated the content of all descendant bundles (See #3414 for reasoning). With bundles maintaining stable hash references through packaging and optimizing, this was overkill, so I updated the method. Unfortunately I updated it to not consider they may end up referencing different bundles 😔.Now all descendant bundle ids are considered in the
BundleGraph.getHash(bundle)function. This is still a little overkill because bundles usually only reference direct child bundles, but in the future when we add mappings of stable ids to final bundle names for all bundles in entry bundles, we would need to consider every child. Maybe the runtime assets with the mappings could add dependencies to all bundles it references, in which case checking only child bundles would be okay. But still, we kind of have a problem that packagers have access to the entireBundleGraphand we aren't totally sure what they are going to do with it. If we are only going to use direct child bundle ids in the hash, we need to make sure that's all packagers have access to. That or add another method to packagers that can narrow down what should go in the cache key before packaging.Another thing I just discovered was that scope hoisting brings in the possibility of bundles referencing parent bundles. They reference not just their names, but their exports too, which means we probably also have to consider parent bundle content in the hash? This is based on my findings working on #4188.
✔️ PR Todo