Skip to content
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

Conversation

pgoldberg
Copy link
Contributor

@pgoldberg pgoldberg commented May 30, 2023

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 5 changed bundle format and now entry modules are concatenated and located at the end of it.
// Because of this they basically become a concatenated module, for which we can't even precisely determine its
// parsed source as it's located in the same scope as all Webpack runtime helpers.

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:

image

After this change, here is what that same view looks like:

image

@pgoldberg pgoldberg marked this pull request as ready for review May 30, 2023 02:55
@pgoldberg pgoldberg marked this pull request as draft May 30, 2023 04:03
@pgoldberg pgoldberg marked this pull request as ready for review May 30, 2023 04:06
@valscion
Copy link
Member

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?

@pgoldberg
Copy link
Contributor Author

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!

@pgoldberg
Copy link
Contributor Author

@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 webpack-bundle-analyzer from Webpack 4 to Webpack 5. On Webpack 4, entry modules that were themselves concatenated modules still showed (inaccurate) parsed and gzipped sizes when that box was ticked.

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 ContentModules.

However, the entry modules themselves can be concatenated modules. In that case, because we've created the module as a ContentModule rather than a ConcatenatedModule, we're not getting estimated sizes for the modules within the concatenated entry module.

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

Comment on lines +25 to +31
getEstimatedSize(sizeType) {
const parentModuleSize = this.parent[sizeType];

if (parentModuleSize !== undefined) {
return Math.floor((this.size / this.parent.size) * parentModuleSize);
}
}
Copy link
Contributor Author

@pgoldberg pgoldberg May 31, 2023

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:

getSize(sizeType) {
const ownerModuleSize = this.ownerModule[sizeType];
if (ownerModuleSize !== undefined) {
return Math.floor((this.size / this.ownerModule.size) * ownerModuleSize);
}
}

Copy link
Member

@valscion valscion left a 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");
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@valscion valscion left a 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 valscion merged commit 59a750c into webpack-contrib:master May 31, 2023
6 checks passed
@pgoldberg
Copy link
Contributor Author

@valscion Thanks for the quick review! 😄

If you could cut a release whenever you get a chance, I'd really appreciate it 🙂

@valscion
Copy link
Member

valscion commented Jun 1, 2023

Yeah ran out of time yesterday, I usually cut releases almost straight away. I'll do a new release now :)

@valscion
Copy link
Member

valscion commented Jun 1, 2023

Released now in v4.9.0! ☺️

@pgoldberg
Copy link
Contributor Author

Thank you!!

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 7, 2023
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#&#8203;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 ([#&#8203;602](webpack-contrib/webpack-bundle-analyzer#602) by [@&#8203;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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants