search

webpack-contrib / webpack-bundle-analyzer

Webpack plugin and CLI utility that represents bundle content as convenient interactive zoomable treemap
MIT License
12.58k stars 488 forks source link

feat: Brotli compression support #432

Open dcsaszar opened 3 years ago

dcsaszar commented 3 years ago

This is my go at #113

Instead of adding Brotli support, I added low-level options to replace Gzip support. The overall approach is inspired by me trying to avoid the hassle of conditional code based on the Node.js version, my own requirements (no need for high-level options), and https://github.com/webpack-contrib/webpack-bundle-analyzer/issues/113#issuecomment-611766511:

I have the use-case that the pipeline analysis doesn't use gzip at all, it relies completely on brotli

Updated with high-level option compressionAlgorithm: gzip | brotli

Example usage:

new BundleAnalyzerPlugin({
  defaultSizes: "compressed",
  compressionAlgorithm: "brotli",
})

Regarding the requirements...

...stated in https://github.com/webpack-contrib/webpack-bundle-analyzer/issues/113#issuecomment-479513104

The only concern I have is the increasing of analyze time. Agree with @valscion here - it would be nice to see a proof-of-concept PR and comparison of analyze times with and without it.

For me, the POC was trying out https://www.npmjs.com/package/webpack-bundle-analyzer-brotli, which works great, but is outdated. The new option is opt-in. Either gzip or Brotli. Users who want Brotli support will be willing to accept the longer wait.

It might be possible to add brotli support if it would be calculated only when some CLI or plugin option would be there. That way it would not increase analyze time for those who don't yet care about Brotli.

The user must specify the compression as an option. They know their environment (e.g. the Node.js version) and resources best.

The crux is that we don't want this to increase analyze time, and this feature would have to be opt-in. Currently it might require some architectural changes, though, so it isn't that easy to do.

I tried to keep the changes at a minimum. In this PR I even left most of the internals (including a lot of gzip* naming) untouched. A cleanup of the internal naming could be done independently, but I'm not sure if this is required.

valscion commented 3 years ago

Thanks for taking a stab at this!

I'm checking out the PR description and the API changes now, and are not focusing on the internal code changes yet as I write this comment, so consider this comment a high-level review ☺️

I'm a bit concerned how this API separetes the compressedSize and compressedSizeLabel options from each other. I'd hope we could have an option like compressionType or compressionAlgorithm instead where one could choose gzip or brotli for now, and it would then automatically set the label.

I can see that we could then later, after this pull request, consider opening up the API more and allow some sort of a compressionAlgorithm: {function} API — and then we'd have to figure out how we want to handle the label in that case.

For now, to ease reviewing, I'd favor a less open API change where we could focus only on choosing between gzip and brotli. We could later discuss the "custom algorithm" option, if we want that.

This "brotli" or "gzip" choice could very well be implemented only at the CLI and Plugin options levels — the internal wiring could keep on using the compressedSize and compressedSizeLabel options you've implemented here. So my comment about only allowing the choice between gzip and brotli would affect only the code in CLI and Plugin options handling, and not the internal wiring.

What do you think? Am I making any sense? 😅

Oh, and as you can see, the tests have become unfortunately quite flaky indeed. I haven't had the time to look into why the tests are timing out or getting aborted. Any help in getting the GitHub actions to run properly would be very much appreciated and would also help reviewing this PR once we're happy with the way the feature has been designed :pray:

We haven't really done anything about the CI since https://github.com/webpack-contrib/webpack-bundle-analyzer/pull/402 where the checks were running successfully. So something has likely changed since then, and it likely isn't anything we've written as there isn't quite much git history since removing Travis CI: https://github.com/webpack-contrib/webpack-bundle-analyzer/commits/master

dcsaszar commented 3 years ago

compressionAlgorithm: 'gzip' | 'brotli' sounds good to me. I'll give it a go.

valscion commented 3 years ago

The CI is now hopefully fixed, thanks to #435. Feel free to merge or rebase newest changes in to this PR to hopefully get a green CI ☺️

dcsaszar commented 3 years ago

@valscion ready for the next review!

dcsaszar commented 3 years ago

@valscion @th0r next round.

valscion commented 3 years ago

I'll let you @th0r review this ☺️ — I think this is heading into a good direction and trust your judgement here :+1:

RodrigoTomeES commented 2 years ago

Hi, Is there any update on this functionality? Thanks for your work!

linux-foundation-easycla[bot] commented 2 years ago

CLA Signed

The committers listed above are authorized under a signed CLA.

pumano commented 1 year ago

Amazing feature not going to production just by personal opinions on how to name it brotli or general word "compression".

dcsaszar commented 1 year ago

Not sure, maybe a misunderstanding about who's turn it is? @th0r could you clarify the to-dos here?

dcsaszar commented 1 year ago

@valscion in https://github.com/webpack-contrib/webpack-bundle-analyzer/pull/432#issuecomment-820386722 you handed the review to @th0r. I see his last PR was #449 so I'm not sure he's still active/in charge. I'd like to continue working on this, but the responsibilities and todos should be clear.

valscion commented 1 year ago

Yeah looks like I can take over reviewing this. Now that brotli is used more and more, having this show in webpack-bundle-analyzer would definitely be useful.

Let's first get this PR up-to-date and then I can start reviewing again. I've lost context of this PR so I'll basically have to start the review process from scratch as well

RuwaidLouisHejare commented 1 year ago

Fantastic to see this PR still is active. I would love to see this in production. Keep up the great work!