-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
improvement: Display more detailed module information for concatenated entry modules #602
improvement: Display more detailed module information for concatenated entry modules #602
Conversation
Huh, that's a neat change with such a small change! Are you able to write up some sort of test to verify this behavior? |
Yep I'll add a test! |
@valscion I added a test, and also updated the PR to include the modules contained in the concatenated entry module's graph for the parsed and gzipped reports, too. After thinking a bit more about this I think this is actually a bug and a regression in On Webpack 5, because we can't determine the exact parsed size of the entry modules, we create a concatenated module containing the entry modules. For those modules, we do get estimated parsed and gzipped sizes, because they're all However, the entry modules themselves can be concatenated modules. In that case, because we've created the module as a I believe this bug fix should only affect entry modules, because this type of situation only really occurs when an entry module is a concatenated module. I don't think webpack would ever output a real concatenated module that is itself a concatenated module – this only happens because of the synthetic concatenated module we create of all the entry modules that we can't accurately parse. The test I added was generated using this minimal repro I made: https://github.com/pgoldberg/webpack-bundle-analyzer-concatenated-entry-modules |
getEstimatedSize(sizeType) { | ||
const parentModuleSize = this.parent[sizeType]; | ||
|
||
if (parentModuleSize !== undefined) { | ||
return Math.floor((this.size / this.parent.size) * parentModuleSize); | ||
} | ||
} |
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 is modified from here:
webpack-bundle-analyzer/src/tree/ContentModule.js
Lines 18 to 24 in a4c304e
getSize(sizeType) { | |
const ownerModuleSize = this.ownerModule[sizeType]; | |
if (ownerModuleSize !== undefined) { | |
return Math.floor((this.size / this.ownerModule.size) * ownerModuleSize); | |
} | |
} |
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.
Looks solid to me!
I have one wish regarding the example case that if possible, would make debugging this behavior later somewhat easier.
Also, could you update the changelog to include this change?
@@ -0,0 +1 @@ | |||
(()=>{"use strict";console.log("side effect"),console.log("side effect")})(),console.log("side effect"); |
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 would be easier to understand which module is which if the test repository you used could use different strings for each module rather than all using console.log("side effect")
😅
I think I do understand how this was generated and where the module boundaries are, though.
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.
Also, if we would name this file app.js
instead of bundle.js
, then we could debug the output more easily also with locally built ./lib/bin/analyzer.js
CLI:
./lib/bin/analyzer.js test/stats/webpack-5-bundle-with-concatenated-entry-module/stats.json
If I change this file name from bundle.js
to app.js
, then I can see the visualization of the bundle contents and can compare that to how current behavior looks like. Without renaming the file, I get this warning:
$ ./lib/bin/analyzer.js test/stats/webpack-5-bundle-with-concatenated-entry-module/stats.json
Error parsing bundle asset ".../webpack-5-bundle-with-concatenated-entry-module/app.js": no such file
No bundles were parsed. Analyzer will show only original module sizes from stats file.
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.
Thanks, looks great!
@valscion Thanks for the quick review! 😄 If you could cut a release whenever you get a chance, I'd really appreciate it 🙂 |
Yeah ran out of time yesterday, I usually cut releases almost straight away. I'll do a new release now :) |
Released now in v4.9.0! |
Thank you!! |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [webpack-bundle-analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer) | devDependencies | minor | [`4.8.0` -> `4.9.0`](https://renovatebot.com/diffs/npm/webpack-bundle-analyzer/4.8.0/4.9.0) | --- ### Release Notes <details> <summary>webpack-contrib/webpack-bundle-analyzer</summary> ### [`v4.9.0`](https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/HEAD/CHANGELOG.md#​490) [Compare Source](webpack-contrib/webpack-bundle-analyzer@v4.8.0...v4.9.0) - **Improvement** - Display modules included in concatenated entry modules on Webpack 5 when "Show content of concatenated modules" is checked ([#​602](webpack-contrib/webpack-bundle-analyzer#602) by [@​pgoldberg](https://github.com/pgoldberg)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMTMuMCIsInVwZGF0ZWRJblZlciI6IjM1LjExMy4wIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCJ9--> Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1925 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Repro of this issue, also used to generate the bundle used in the test: https://github.com/pgoldberg/webpack-bundle-analyzer-concatenated-entry-modules
After migrating a large monorepo from Webpack 4 to Webpack 5, we noticed that we lost detailed entry module information, which made the bundle analyzer reports much less useful for us. After looking a little bit through the
webpack-bundle-analyzer
repo, I found this comment:webpack-bundle-analyzer/src/analyzer.js
Lines 124 to 126 in e231c77
Although we can't determine the parsed sizes of the concatenated modules, we should show the estimated parsed and gzipped sizes for the concatenated entry modules like we do for other concatenated modules. More explanation of the issue here: #602 (comment)
Here's a before and after:
After this change, here is what that same view looks like: