Repository navigation
fix: prevent TypeError when child compilations have missing assets - #741
Conversation
🦋 Changeset detectedLatest commit: 3e459b1 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. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to For compilation-array inputs with repeated asset filenames, a later asset replaces an earlier one in the chart, which can omit a bundle from reports. This is limited to same-name outputs; the per-compilation entrypoint mapping concern is addressed. 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). 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
==========================================
+ Coverage 87.56% 89.04% +1.47%
==========================================
Files 17 17
Lines 1110 1132 +22
Branches 406 420 +14
==========================================
+ Hits 972 1008 +36
+ Misses 126 115 -11
+ 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.
|
@valscion can you review it please I have made the changes |
08c65cf to
3e459b1
Compare
valscion
left a comment
valscion
left a comment
There was a problem hiding this comment.
Yeah looks good to me! 👍
Have these example stats objects been generated via some real-life webpack bundle contents or how did you come up with those test cases?
|
They are minimal synthetic unit-test fixtures modeled after the existing inline stats fixture at the top of test/analyzerUtils.js and the schemas from the existing test/stats/ fixtures. I designed each one around the specific shapes that trigger the bug in #490 and the related code paths in getViewerData. Keeping them minimal in test/analyzerUtils.js made it easy to isolate each edge case without large JSON fixtures |
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