Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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 brotli size support #113

Closed
aldo-roman opened this issue Sep 6, 2017 · 14 comments
Closed

Add brotli size support #113

aldo-roman opened this issue Sep 6, 2017 · 14 comments

Comments

@aldo-roman
Copy link

aldo-roman commented Sep 6, 2017

This is a discussion issue to add Brotli size support on the report, side by side with gzip.
On the app we support, 78% of users surf the web with a brotli-enabled browser.
Global support is around 60%.
This will increase in the next days after macOS High Sierra and iOS 11 are launched.
See http://caniuse.com/#search=brotli

Also, in our tests, Brotli grants 10% more compression than Gzip.
We are developing a Angular CLI, Angular 4+ app.

I will be experimenting with a fork to add Brotli by replicating the Gzip size implementation.
Any suggestion and feedback is welcome.

@valscion
Copy link
Member

valscion commented Sep 7, 2017

Huh, never heard of Brotli before 😄 . Awesome to learn new things!

@th0r should be able to answer with any suggestions and feedback, when he's got time. I'd like to at least see a proof-of-concept of this Brotli support if possible ☺️

@th0r
Copy link
Collaborator

th0r commented Sep 7, 2017

Never heard of Brotli as well. 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.

@valscion
Copy link
Member

valscion commented Sep 7, 2017

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.

@th0r
Copy link
Collaborator

th0r commented Sep 7, 2017

👍

@aldo-roman
Copy link
Author

Hi guys, here there is the PR for Brotli support. It is working without problems on my setup. I couldn't include an opt-in option for the feature.

Here is a screenshot on how this looks:
image

@valscion
Copy link
Member

Neat! I think it'll be necessary to have an opt-in somehow, but currently it might require some architectural changes. Large changes would cause difficult merge conflicts with #97, though — I hope I could get that finished some day 😕

I hope you're OK with using the Brotli support from your branch? It's a bit unlikely that we'll want to calculate these sizes by default as brotli isn't that widely used yet

@edmorley
Copy link
Contributor

edmorley commented May 15, 2018

If this does get implemented, please can the brotli related dependencies not be added to dependencies - to avoid issues like those in siddharthkp/bundlesize#202. (eg check for the dependencies at runtime and if not found and "brotli mode" enabled, output a suitable message.)

Re Brotli itself, we happily use it on several production sites, however I'm not sure of the value of displaying brotli sizes in the analyzer, given:
(a) brotli compression is much slower so would have much more of an impact on how long this plugin takes to run,
(b) whilst not entirely linear, one would presume the size ratios of the bundle for gzip vs brotli would be close enough that it wouldn't really affect any decisions based on the results (eg deciding what changes to make to splitChunks settings).

@valscion
Copy link
Member

If this does get implemented, please can the brotli related dependencies not be added to dependencies - to avoid issues like those in siddharthkp/bundlesize#202. (eg check for the dependencies at runtime and if not found and "brotli mode" enabled, output a suitable message.)

Huh, thanks for the pointer :)

@manniL
Copy link

manniL commented Jan 18, 2019

That should be easier now that native Brotli support (https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V11.md#notable-changes) landed in Node.

@salztorte
Copy link

Something new to this?

@valscion
Copy link
Member

valscion commented Apr 3, 2019

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

#113 (comment)

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.

#114 (comment)

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.

@278kunal
Copy link

When is this getting merged ?

@valscion
Copy link
Member

When someone who wants brotli size support does what we've described that needs to be done. See the comment right above yours.

@raphaelbs
Copy link

I have the use-case that the pipeline analysis doesn't use gzip at all, it relies completely on brotli. It would be great if it had the ability to choose one of them.

@webpack-contrib webpack-contrib locked and limited conversation to collaborators Feb 23, 2023
@valscion valscion converted this issue into discussion #562 Feb 23, 2023

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants