Use stable ids in names of inline bundles #4188
Closed
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
devongovett,
wbinnssmith,
mischnic and
DeMoorJasper
wbinnssmith
reviewed
Feb 20, 2020
| @@ -91,7 +91,8 @@ export default new Namer({ | |||
| // `index.css`. | |||
| let name = nameFromContent(mainBundle, options.rootDir); | |||
| if (!bundle.isEntry) { | |||
| name += '.' + bundle.hashReference; | |||
| let hash = bundle.isInline ? bundle.id.slice(-8) : bundle.hashReference; | |||
wbinnssmith
approved these changes
Feb 20, 2020
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
|
This was referenced Feb 20, 2020
mischnic
requested changes
Feb 20, 2020
There was a problem hiding this comment.
I just tested this with the repro in the linked issue, but I think the issue is bigger:
The blue JS bundle imports the pink JS bundle, but the pink JS bundle is inlined into the HTML asset, so it actually cannot be imported.
<!-- index.html -->
<script type="module">
export var $d8762107a9da24ea5810e689596fa963$exports = {};
// ...
import("" + "./options.6b63f96a.js");
</script>// options.6b63f96a.js
import { $d8762107a9da24ea5810e689596fa963$exports }
from "./4145-hashReferences-scope-hoist.1370e440.js"; // the inline bundle above, doesn't exist
console.log("options", $d8762107a9da24ea5810e689596fa963$exports.default);
|
@mischnic Yeah that seemed weird to me, that's why I asked if it was expected. So it's scope hoisting bug? Should it be importing the bundle that contains the inline bundle instead? |
And/or a bundler bug...
|
|
I am going to close this as the right solution is probably to not name inline bundles at all (See #4172) |
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
Fixes #4145
The issue was caused by a bundle that contained a hash reference to an inline parent bundle, which is something I didn't anticipate, but is apparently needed for scope hoisting. @devongovett @mischnic Is this expected?
See below for a visual of the bundle graph. In this case the blue bundle is referencing the pink bundle.
To fix this, I changed the default namer to not use content hash references in inline bundle names. It still uses some characters of the bundle id to ensure uniqueness. This seems like a safe option since inline bundles are not served by themselves so their names do not need to be long term cacheable.
🚨 Test instructions
Reproduced the failure in the linked issue and tested that the changes were able to build the code successfully. We should probably add some integration tests, but I wanted to make sure that this scenario is expected.
✔️ PR Todo