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 brotli support #80

Closed
wants to merge 2 commits into from
Closed

Add brotli support #80

wants to merge 2 commits into from

Conversation

tracker1
Copy link

Closes #21, full version bump because of behavioral change, still backwards compatible though.

Only enabled brotli if the optional dependency of iltorb is available and br option is specified. Added notes in the readme, as well as a suggestion to only use in conjunction with an output cache.

In my own tests:

encoding  level/quality  time   size
gzip      9              36ms   129kb
br        11             441ms  127kb
br        8              43ms   128kb
br        6              38ms   128kb
br        5              36ms   128kb

@tracker1 tracker1 mentioned this pull request Aug 13, 2018
@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #80 into master will decrease coverage by 7.31%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
- Coverage     100%   92.68%   -7.32%     
==========================================
  Files           1        1              
  Lines          32       41       +9     
  Branches       15       17       +2     
==========================================
+ Hits           32       38       +6     
- Misses          0        3       +3
Impacted Files Coverage Δ
index.js 92.68% <91.89%> (-7.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92f6ff0...d6ef4ad. Read the comment docs.

@tracker1
Copy link
Author

Looks like travis borked, and not sure about updating the tests... I implemented out of my own curiousity.

@curbengh
Copy link

curbengh commented Oct 3, 2019

brotli support has been added to node >= 10.16. native brotli can be prioritized over iltorb when supported.

See expressjs/compression#158 expressjs/compression#156


Just stumbled upon https://github.com/tuananh/kompression which is a fork with brotli support.

@uhop uhop mentioned this pull request Nov 4, 2019
@jonathanong
Copy link
Member

@tracker1 sorry about the delay. going in favor of #92 since it is using native node APIs. please review it!

@tracker1
Copy link
Author

tracker1 commented May 2, 2020

@jonathanong that's cool... my PR predates native support iirc... I did follow the other PR, and lgtm.

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.

Support for brotli
3 participants