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

Support for Brotli compression #71

Open
joostdebruijn opened this issue Jan 24, 2016 · 17 comments · May be fixed by #173
Open

Support for Brotli compression #71

joostdebruijn opened this issue Jan 24, 2016 · 17 comments · May be fixed by #173
Assignees

Comments

@joostdebruijn
Copy link

Is it an idea to add Brotli compression for https-requests to this module? Now support for Brotli has landed in the developer channels of Firefox and Chrome it is very likely that a significantly amount of clients will have support for this enabled in the very near future. So, it is maybe a good idea to support it in this module too.

I’m not (yet) very experienced with building modules for Node, but if there is anything I can help with, please let me know.

@dougwilson
Copy link
Contributor

Hi! This was discussed before in #59 if you want to read the prior discussion.

The answer is that we certainly can, but for it to be built-in here, we need the support either added to Node.js core or a pure-JS library.

Barring that, another option is to make this module flexible and allow for alternative user-defined encodings. There was the PR #62 but it was never completed due to a disagreement between the author and myself over a feature I wanted to be supported.

@dantman
Copy link

dantman commented Feb 1, 2016

The performance of brotli is something that should be kept in mind.

Some previous tests:

brotli isn't necessarily always more efficient than gzip. brotli's biggest advantage is that at higher compression levels, at the cost of compression time, you can get compression ratios beyond what you can do even by using zopfli.

So you don't really get an advantage from using zopfli or brotli for dynamic content. They work best when you can wait a few extra seconds to let the compressor do more work.

For most people this means making a .gz file (using zopfli) and a .bro file with brotli during your build step and using the pre-compressed files instead of dynamically compressing with either.

@LeandroFavero
Copy link

dougwilson, maybe this can help:
A JavaScript port of the Brotli compression algorithm
https://github.com/devongovett/brotli.js

@passcod
Copy link

passcod commented May 25, 2016

Note that the above port of Brotli doesn't support streaming yet. People interested in brotli and zopfli in the meantime may want to have a look at https://github.com/aickin/shrink-ray.

@mikemaccana
Copy link

mikemaccana commented Apr 4, 2017

@dantman At level 4, Brotli uses less CPU AND compresses better than gzip - here's the results of a test using the top 1000 URLs on the internet: https://blogs.akamai.com/2016/02/understanding-brotlis-potential.html

@dougwilson
Copy link
Contributor

It looks like Node.js is discussing adding support ad a built-in nodejs/node#18964 which should make it trivial to add to this module.

@yocontra
Copy link

This library supports streaming - https://github.com/MayhemYDG/iltorb#stream (native package though, maybe make it an optional dep?)

If the maintainers are interested I can send a PR, let me know.

@voxpelli
Copy link

voxpelli commented Jan 7, 2019

Built in support in Node landed 2 days ago: nodejs/node#24938

@adrienharnay
Copy link

adrienharnay commented Jan 18, 2019

Node 11.7.0 has been released with brotli support 🎉

@dougwilson
Copy link
Contributor

PR #150

@camillegrings
Copy link

Hey guys, when are you going to release the new version with support for brotli? I'm already using your lib, and i didn't wanted to change to another, because your lib is working fine in my project.

@patrickmichalina
Copy link

I submitted a PR #156 since there was not progress on the other one.

@KB1RMA
Copy link

KB1RMA commented Aug 7, 2019

Is there any reason Patrick's PR from above is still waiting for a merge? I'm running on 10.16 at the moment and would love to be able to turn brotli on - I can't see anything obvious holding up the release of that option via the PR.

@dougwilson
Copy link
Contributor

It seems like a duplicate of #150 and doesn't even address the issues that were brought up in that issue.

@KB1RMA
Copy link

KB1RMA commented Aug 10, 2019

Ah I missed that PR, sorry. Looks like it addresses the testing well but not the options.

@patrickmichalina
Copy link

I can look into the options portion as well.

@expressjs expressjs deleted a comment from cupcakearmy May 21, 2020
@nicksrandall nicksrandall linked a pull request Sep 15, 2020 that will close this issue
@nicksrandall
Copy link

Yet another PR #173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.