feat: Brotli compression support #432

dcsaszar · 2021-04-10T19:33:39Z

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 #113 (comment):

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 #113 (comment)

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 · 2021-04-12T07:42:41Z

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 🙏

We haven't really done anything about the CI since #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 · 2021-04-12T12:41:36Z

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

valscion · 2021-04-13T08:05:20Z

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 ☺️

To be in line with the new `compressedSize*` options. Supports `gzip` as a synonym for backwards compatibility. Internal naming is also still `gzip*`.

dcsaszar · 2021-04-14T23:39:31Z

@valscion ready for the next review!

valscion

I like the direction of this PR 👍. It was a good idea to get rid of the gzip-size package and rely on the native zlib module also for the gzip case — it makes the code paths for brotli and gzip look a bit more similar, and all the previous usages of gzip-size were easy to find 👍

src/sizeUtils.js

valscion · 2021-04-15T06:34:47Z

test/plugin.js

+        await webpackCompile(config, '4.44.2');
+        await expectValidReport({
+          parsedSize: 1311,
+          gzipSize: 302


I think that, even though it increases the size of this PR, it might be better to aim to use brotliSize in the data when Brotli is used instead of gzipSize. It would be quite confusing for direct users of the report JSON file (e.g. those who supply --analyzerMode json value to CLI) if the key would remain as gzipSize even though Brotli was selected.

What do you think? Does the confusion risk outweigh changing the code more?

Maybe just compressedSize?

@th0r I thought about this too, as I'm not a friend of key explosion.

On the other hand I'm OK with gzipSize, brotliSize:

backwards compatible

unambiguous for (human/bot) readers of the raw data

theoretically allows for dual-compression stats

valscion · 2021-04-15T06:40:42Z

src/viewer.js

@@ -126,21 +136,23 @@ async function generateReport(bundleStats, opts) {
    openBrowser = true,
    reportFilename,
    reportTitle,
+    compressionAlgorithm = 'gzip',


Oh yeah, I see you've found that we have multiple places where we're defining default values for options, and haven't really made up our mind where they'd belong 😅

How do you feel about moving all the compressionAlgorithm = 'gzip default value definitions to the top-level usages? That is, moving the compressionAlgorithm = 'gzip' code to the plugin starting point in src/BundleAnalyzerPlugin.js. The CLI starting point in src/bin/analyzer.js already seems to define the default value for the --compression-algorithm to be gzip so we should be covered there.

Then we could get rid of all the compressionAlgorithm = 'gzip' defaulting in other code paths and assume that we have the value filled in in those code paths.

valscion · 2021-04-15T06:46:53Z

README.md

-### `gzip`
+### `compressed`


Can we make it so that the added option for defaultSizes would be brotli instead of compressed? That way the current, default gzip usage would not have to be changed.

That way we could keep the gzip-related documentation intact and only worry about adding new documentation related to the brotli options.

I could imagine that the fourth size definition documentation here could look something like this:

brotli

This is the size of running the parsed bundles/modules through Brotli compression. Note: This needs the compression algorithm to be configured to use Brotli.

th0r · 2021-04-15T09:32:17Z

client/components/ModulesTreemap.jsx

+  return [
+    {label: 'Stat', prop: 'statSize'},
+    {label: 'Parsed', prop: 'parsedSize'},
+    {label: window.compressedSizeLabel, prop: 'gzipSize'}


Can we change gzipSize to compressedSize?

th0r · 2021-04-15T09:36:04Z

src/analyzer.js

@@ -102,7 +111,7 @@ function getViewerData(bundleStats, bundleDir, opts) {

    if (assetSources) {
      asset.parsedSize = Buffer.byteLength(assetSources.src);
-      asset.gzipSize = gzipSize.sync(assetSources.src);
+      asset.gzipSize = compressedSize(assetSources.src);


?

Suggested change

asset.gzipSize = compressedSize(assetSources.src);

asset.compressedSize = compressedSize(assetSources.src);

th0r · 2021-04-15T09:36:53Z

src/analyzer.js

  } = opts || {};

  const isAssetIncluded = createAssetsFilter(excludeAssets);

+  const compressedSize = COMPRESSED_SIZE[compressionAlgorithm];


Let's better call it getCompressedSize please

th0r · 2021-04-15T09:39:35Z

src/analyzer.js

@@ -143,7 +152,7 @@ function getViewerData(bundleStats, bundleDir, opts) {
    }

    asset.modules = assetModules;
-    asset.tree = createModulesTree(asset.modules);
+    asset.tree = createModulesTree(asset.modules, {compressedSize});


Suggested change

asset.tree = createModulesTree(asset.modules, {compressedSize});

asset.tree = createModulesTree(asset.modules, {getCompressedSize});

th0r · 2021-04-15T09:46:25Z

src/viewer.js

@@ -22,6 +22,14 @@ function resolveTitle(reportTitle) {
  }
 }

+function resolveDefaultSizes(defaultSizes) {
+  return defaultSizes === 'compressed' ? 'gzip' : defaultSizes;


Can we inverse it please? I mean, deprecate gzip default size and use compressed instead so it will look like this:

Suggested change

return defaultSizes === 'compressed' ? 'gzip' : defaultSizes;

return defaultSizes === 'gzip' ? 'compressed' : defaultSizes;

My current approach, based on #432 (comment), is:

function resolveDefaultSizes(defaultSizes, compressionAlgorithm) { if (['gzip', 'brotli'].includes(defaultSizes)) return compressionAlgorithm; return defaultSizes; }

That is, we drop compressed. The config is "forgiving" if the user specifies the wrong defaultSizes.
WDYT?

but be forgiving with a gzip/brotli mismatch

dcsaszar · 2021-04-15T10:15:08Z

@valscion @th0r next round.

th0r · 2021-04-15T10:25:01Z

README.md

@@ -61,7 +61,8 @@ new BundleAnalyzerPlugin(options?: object)
 |**`analyzerPort`**|`{Number}` or `auto`|Default: `8888`. Port that will be used in `server` mode to start HTTP server.|
 |**`reportFilename`**|`{String}`|Default: `report.html`. Path to bundle report file that will be generated in `static` mode. It can be either an absolute path or a path relative to a bundle output directory (which is output.path in webpack config).|
 |**`reportTitle`**|`{String\|function}`|Default: function that returns pretty printed current date and time. Content of the HTML `title` element; or a function of the form `() => string` that provides the content.|
-|**`defaultSizes`**|One of: `stat`, `parsed`, `gzip`|Default: `parsed`. Module sizes to show in report by default. [Size definitions](#size-definitions) section describes what these values mean.|
+|**`defaultSizes`**|One of: `stat`, `parsed`, `gzip`, `brotli`|Default: `parsed`. Module sizes to show in report by default. [Size definitions](#size-definitions) section describes what these values mean.|


In this case user will have to provide compression algorithm twice: for compressionAlgorithm and defaultSizes and it opens possibilities for invalid option combinations like defaultSizes: 'gzip', compressionAlgorithm: 'brotli' which don't make sense. I personally would change defaultSizes: 'gzip' to defaultSizes: 'compressed' and deprecate the former, but support it (convert to compressed).

The current implementation handles the mismatch gracefully, so it's more of a cosmetic issue on the user side.

Reintroducing compressed would help here. If used, the config always looks "good", even when switching the algorithm. If we support compressed, fail on mismatches between compressionAlgorithm and defaultSizes?

th0r · 2021-04-15T10:29:39Z

client/components/ModulesTreemap.jsx

+    {label: 'Parsed', prop: 'parsedSize'}
+  ];
+
+  if (window.compressionAlgorithm === 'gzip') items.push({label: 'Gzipped', prop: 'gzipSize'});


I would change gzipSize/brotliSize to compressedSize but looks like it will be a breaking change.

Would be nice if we could first do this as a new feature, release in a minor version and then do a major release later to consolidate the API on on plain compressedSize. That has been my opinion during this PR review at least ☺️.

What do you think of this plan?

valscion · 2021-04-15T12:33:28Z

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

RodrigoTomeES · 2022-07-31T10:23:53Z

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

linux-foundation-easycla · 2022-09-06T18:01:38Z

The committers listed above are authorized under a signed CLA.

✅ login: dcsaszar / name: David Császár (912aa8c, 6a4a361, ec1fa85, 4dfbb5c, 80511a8, 70d8cf1, 3f183c2, b8dcd04, b71f9ae, 13f00fe, db1d33d)

pumano · 2023-08-04T15:36:29Z

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

dcsaszar · 2023-08-04T16:05:49Z

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

dcsaszar · 2023-08-13T12:05:46Z

@valscion in #432 (comment) 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 · 2023-08-14T06:21:55Z

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 · 2023-08-22T08:18:15Z

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

dcsaszar force-pushed the dave-custom-compression-support-aka-brotli branch from e9d87bf to 32dc3e7 Compare April 10, 2021 19:49

valscion mentioned this pull request Apr 12, 2021

Fixes #417 missing module chunks #433

Merged

deanshub mentioned this pull request Apr 13, 2021

Fix tests timeout #435

Merged

dcsaszar added 4 commits April 13, 2021 10:38

Drop gzip-size dependency

912aa8c

Single source of thruth for compressed size helper

6a4a361

Change public API from gzip to compressed

ec1fa85

To be in line with the new `compressedSize*` options. Supports `gzip` as a synonym for backwards compatibility. Internal naming is also still `gzip*`.

Add Brotli size helper

4dfbb5c

dcsaszar force-pushed the dave-custom-compression-support-aka-brotli branch from 32dc3e7 to 85a6376 Compare April 14, 2021 23:19

dcsaszar added 2 commits April 15, 2021 01:32

Add compressionAlgorithm option

80511a8

Add compression algorithm options to README

70d8cf1

dcsaszar force-pushed the dave-custom-compression-support-aka-brotli branch from 85a6376 to 70d8cf1 Compare April 14, 2021 23:33

dcsaszar changed the title ~~feat: Custom compression support (a.k.a. Brotli)~~ feat: Brotli compression support Apr 14, 2021

valscion reviewed Apr 15, 2021

View reviewed changes

Separate gzip and brotli data

3f183c2

th0r requested changes Apr 15, 2021

View reviewed changes

dcsaszar added 2 commits April 15, 2021 11:56

compressedSize => getCompressedSize

b8dcd04

Drop defaultSizes: 'compressed'

b71f9ae

but be forgiving with a gzip/brotli mismatch

dcsaszar requested review from valscion and th0r April 15, 2021 10:15

th0r requested changes Apr 15, 2021

View reviewed changes

dcsaszar added 2 commits September 6, 2022 19:40

Merge branch 'master' into dave-custom-compression-support-aka-brotli

13f00fe

Update brotli analyzer tests after merge

db1d33d

valscion mentioned this pull request Oct 17, 2023

Shows brotli size in report #504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Brotli compression support #432

feat: Brotli compression support #432

dcsaszar commented Apr 10, 2021 •

edited

valscion commented Apr 12, 2021

dcsaszar commented Apr 12, 2021

valscion commented Apr 13, 2021

dcsaszar commented Apr 14, 2021

valscion left a comment

valscion Apr 15, 2021

th0r Apr 15, 2021

dcsaszar Apr 15, 2021 •

edited

valscion Apr 15, 2021

valscion Apr 15, 2021

th0r Apr 15, 2021

th0r Apr 15, 2021

th0r Apr 15, 2021

th0r Apr 15, 2021

th0r Apr 15, 2021 •

edited

dcsaszar Apr 15, 2021

dcsaszar commented Apr 15, 2021

th0r Apr 15, 2021

dcsaszar Apr 15, 2021

th0r Apr 15, 2021

valscion Apr 15, 2021

valscion commented Apr 15, 2021

RodrigoTomeES commented Jul 31, 2022

linux-foundation-easycla bot commented Sep 6, 2022 •

edited

pumano commented Aug 4, 2023

dcsaszar commented Aug 4, 2023 •

edited

dcsaszar commented Aug 13, 2023

valscion commented Aug 14, 2023

RuwaidLouisHejare commented Aug 22, 2023 •

edited

	asset.gzipSize = compressedSize(assetSources.src);
	asset.compressedSize = compressedSize(assetSources.src);

	asset.tree = createModulesTree(asset.modules, {compressedSize});
	asset.tree = createModulesTree(asset.modules, {getCompressedSize});

	return defaultSizes === 'compressed' ? 'gzip' : defaultSizes;
	return defaultSizes === 'gzip' ? 'compressed' : defaultSizes;

		### `gzip`
		### `compressed`

feat: Brotli compression support #432

Are you sure you want to change the base?

feat: Brotli compression support #432

Conversation

dcsaszar commented Apr 10, 2021 • edited

Example usage:

Regarding the requirements...

valscion commented Apr 12, 2021

dcsaszar commented Apr 12, 2021

valscion commented Apr 13, 2021

dcsaszar commented Apr 14, 2021

valscion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcsaszar Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brotli

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th0r Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcsaszar commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valscion commented Apr 15, 2021

RodrigoTomeES commented Jul 31, 2022

linux-foundation-easycla bot commented Sep 6, 2022 • edited

pumano commented Aug 4, 2023

dcsaszar commented Aug 4, 2023 • edited

dcsaszar commented Aug 13, 2023

valscion commented Aug 14, 2023

RuwaidLouisHejare commented Aug 22, 2023 • edited

dcsaszar commented Apr 10, 2021 •

edited

dcsaszar Apr 15, 2021 •

edited

`brotli`

th0r Apr 15, 2021 •

edited

linux-foundation-easycla bot commented Sep 6, 2022 •

edited

dcsaszar commented Aug 4, 2023 •

edited

RuwaidLouisHejare commented Aug 22, 2023 •

edited