fix: prevent TypeError when child compilations have missing assets - #741
ronakmaheshwari wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: a5ab3a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@valscion can you review my PR please |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Walkthrough
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Charts for multiple compilations can misidentify later entrypoint assets. Preserve each asset’s entrypoint metadata before merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/analyzer.jsESLint failed to execute (timeout). test/analyzerUtils.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve duplicate asset entries across compilation arrays. · analyzer.js:445-453
src/analyzer.js:445-453
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve duplicate asset entries across compilation arrays.
The array-input path appends assets from each compilation, but the reducer stores them in a filename-keyed object. If two compilations emit the same filename,
result[statAsset.name]replaces the earlier asset. The returned chart data then contains only the later entry.Store the intermediate assets as records in an array so duplicate filenames retain separate chart entries.
Suggested fix
- const asset = (result[statAsset.name] = /** `@type` {Asset} */ ({ + const asset = /** `@type` {Asset} */ ({ size: statAsset.size, - })); + }); + result.push({ filename: statAsset.name, asset }); ... - }, /** `@type` {Record<string, Asset>} */ ({})); + }, /** `@type` {{ filename: string, asset: Asset }[]} */ ([])); ... - return Object.entries(assets).map(([filename, asset]) => ({ + return assets.map(({ filename, asset }) => ({
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d71a52c3-c89f-40bd-aff1-15869d64383a
📒 Files selected for processing (3)
.changeset/fix-child-compilations-type-error.mdsrc/analyzer.jstest/analyzerUtils.js
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
==========================================
+ Coverage 87.56% 88.80% +1.24%
==========================================
Files 17 17
Lines 1110 1126 +16
Branches 406 421 +15
==========================================
+ Hits 972 1000 +28
+ Misses 126 117 -9
+ Partials 12 9 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
valscion
left a comment
valscion
left a comment
There was a problem hiding this comment.
Looks good to me! The .changeset/ needs actual content, I don't know why that file ended up being a 0 byte file here.
Check if the coderabbit thing is something we need to address here. It doesn't look like an issue to me but I think you might be better equipped to judge it.
… improve coverage
|
Thanks for the review, @valscion! I've pushed updates addressing both points:
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 38c666e9-b000-4e83-b181-e0b426584f61
📒 Files selected for processing (3)
.changeset/fix-child-compilations-type-error.mdsrc/analyzer.jstest/analyzerUtils.js
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| for (let i = 1; i < children.length; i++) { | ||
| for (const asset of children[i].assets || []) { | ||
| for (let i = 1; i < allChildren.length; i++) { | ||
| const child = allChildren[i]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve entrypoint metadata for appended compilations.
When an array contains compilations with different asset names, these loops append the later assets but do not preserve their entrypoints. getChunkToInitialByEntrypoint(bundleStats) then reads only the first compilation. A later compilation’s entrypoint asset receives an empty isInitialByEntrypoint map. Build that metadata from each asset’s source compilation in both append paths.
Also applies to: 319-319
2 participants
What kind of change does this PR introduce?
Bug fix and test coverage improvement.
This PR improves the robustness of
getViewerDataand related functions when handling edge cases in Webpack stats, particularly child compilations with missing or undefined assets, stats provided as arrays, and differences in asset formats between Webpack versions.It also improves child asset/module mapping and adds compatibility for Webpack 4 entrypoints where assets may be represented as strings.
Did you add tests for your changes?
Yes.
Added tests covering:
assetsDoes this PR introduce a breaking change?
No.
The changes are intended to improve handling of existing Webpack stats structures without changing the expected behavior for supported inputs.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
No documentation changes are required.
This PR is an internal robustness and compatibility improvement and does not introduce any new user-facing configuration or API.
Use of AI
No.
#490
Summary by CodeRabbit