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

Add ability to filter to only initial JS by entrypoint #519

Merged

Conversation

pas-trop-de-zele
Copy link
Contributor

@pas-trop-de-zele pas-trop-de-zele commented Aug 15, 2022

Context

At the moment, webpack-bundle-analyzer allows filtering bundles by chunks which is very nice since that allows us to see what’s really inside our bundle and do appropriate optimization.

However, there is not really a way for us to look at the initial JS being loaded. As a result, we added a filter to enable the ability to filter down to the initial chunks by entrypoint. This change would allow us to see the JS being loaded on the page before any dynamic imports are executed, which in lots of cases is responsible for app slow loading time

Changes

Added new isInitialByEntrypoint field to chunks object in chartData. This is a nested object with key being entrypoints and value being boolean if the chunk is initial for that entry point

chartData: [
  {
    label: ‘chunkName’,
    isAsset: true,
    …,
    isInitialByEntrypoint: {
      {entrypointA: true},
      {entrypointB: true},
    }
  },
]

The reason for choosing nested object with boolean value instead of a more straightforward Set approach is that Set cannot be serialized and chartData would have to be put on the windows object. The nested object option could be serialized while keeping the search performant

Screenshot

Demo

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 15, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@pas-trop-de-zele pas-trop-de-zele force-pushed the samle/addInitialJsToGraph branch 2 times, most recently from eebdb15 to 5679ca9 Compare August 15, 2022 21:51
@pas-trop-de-zele pas-trop-de-zele changed the title test Add ability to filter to only initial JS by entrypoint Aug 16, 2022
@valscion
Copy link
Member

Oh nice, this looks like a really useful feature! Thank you for the PR ☺️

Do you think it's possible to also add test coverage going from a webpack stats JSON object to the end result? So that the test would look similar to e.g. what we have here:

it('should support generating JSON output for the report', async function () {
generateJSONReportFrom('with-modules-in-chunks/stats.json');
const chartData = require(path.resolve(__dirname, 'output/report.json'));
expect(chartData).to.containSubset(require('./stats/with-modules-in-chunks/expected-chart-data'));
});

It would be nice if we could test the end result rather than only the getChunkToInitialByEntrypoint function behavior itself. Ideally we would not even need to export the getChunkToInitialByEntrypoint function from src/analyzer.js but keep that function as an implementation detail. Right now only the tests need that function to be exported, and it would be nice if the tests could avoid that but still get same coverage.

@pas-trop-de-zele
Copy link
Contributor Author

@valscion Changed test format like you request

@valscion
Copy link
Member

Thanks! I'll take this PR for a test drive when I get a good slot some point in the future. I didn't spot any glaring issues when reading over the code quickly, so that's good. I'll review more closely when I have the time

@gluxon
Copy link

gluxon commented Aug 24, 2022

Thanks for taking a look @valscion. 🙂 I'm on @pas-trop-de-zele's team and we're glad to hear it'll be useful feature.

@pgoldberg
Copy link
Contributor

Hey @valscion, just wanted to bump this to see if you'd get a chance to take a look, since this would be a really useful feature for us. I'd be happy to make any changes to the implementation if necessary. Thank you!

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.

The code looks good enough to me! Are you able to add a changelog entry? And also fix the typo spotted in test code ☺️

test/viewer.js Outdated Show resolved Hide resolved
@valscion
Copy link
Member

I was also able to test this locally and this appears to be working as intended ☺️

Co-authored-by: Vesa Laakso <482561+valscion@users.noreply.github.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@pgoldberg
Copy link
Contributor

Thanks so much for taking a look @valscion! I added a changelog and fixed the typo – let me know if there's any other changes you'd like us to make before merging 🙂

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 good to me, thanks!

CHANGELOG.md Outdated
@@ -12,6 +12,9 @@ _Note: Gaps between patch versions are faulty, broken or test releases._

## UNRELEASED

* **Improvement**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd classify this as a "New Feature", it's a nice addition! ☺️

@valscion valscion merged commit 8a3d3f0 into webpack-contrib:master Oct 26, 2022
@valscion
Copy link
Member

This is now released in v4.7.0, thank you for your contributions! 🚀

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Oct 26, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [webpack-bundle-analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer) | devDependencies | minor | [`4.6.1` -> `4.7.0`](https://renovatebot.com/diffs/npm/webpack-bundle-analyzer/4.6.1/4.7.0) |

---

### Release Notes

<details>
<summary>webpack-contrib/webpack-bundle-analyzer</summary>

### [`v4.7.0`](https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/HEAD/CHANGELOG.md#&#8203;470)

[Compare Source](webpack-contrib/webpack-bundle-analyzer@v4.6.1...v4.7.0)

-   **New Feature**
    -   Add the ability to filter to displaying only initial chunks per entrypoint ([#&#8203;519](webpack-contrib/webpack-bundle-analyzer#519) by [@&#8203;pas-trop-de-zele](https://github.com/pas-trop-de-zele))

</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, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xLjIiLCJ1cGRhdGVkSW5WZXIiOiIzNC4xLjIifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1608
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants