Added native Brotli support. #92

uhop · 2019-11-04T05:16:08Z

Native support if Brotli is available in Node.
Separate options via brotliOptions.
brotliOptions is null => skip Brotli.
New CI targets to test more platforms.
README is amended with the Brotli-specific docs.
New comprehensive tests for all new functionality.

* Native support if Brotli is available in Node. * Separate options via `brotliOptions`. * `brotliOptions` is `null` => skip Brotli. * New CI targets to test more platforms. * README is amended with the Brotli-specific docs. * New comprehensive tests for all new functionality.

uhop · 2019-11-04T05:21:00Z

This is a minimalistic PR for Brotli with all docs and tests. It is backward compatible, allows suppressing br completely or controlling it with options.

It will take care of #21 and will bring us in line with modern versions of Node (Brotli works starting from Node 10, we are at 13 now).

codecov · 2019-11-04T05:23:46Z

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #92   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           32        36    +4     
  Branches        15        19    +4     
=========================================
+ Hits            32        36    +4

Impacted Files	Coverage Δ
index.js	`100.00% <100.00%> (ø)`

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 56f0600...ad8a70e. Read the comment docs.

uhop · 2019-11-04T05:31:42Z

Just in case — differences with #80:

No new external dependencies whatsoever.
Backward compatible.
Trivial to turn off Brotli, if you want the 100% old behavior.
All tests are passed including style and coverage.

markulrich · 2020-02-06T22:16:46Z

@jonathanong is this safe to land? Would love to have Brotli support for Every.org!

tracker1 · 2020-02-07T20:20:21Z

If you don't have a caching layer above this, be careful... Brotli is much slower than gz if you want better compression, and even if you relax the compression level, still slower than higher level gz for less compression. Brotli is generally best for ahead of time compression of static assets.

I went through the trouble of creating a brotli fork myself to work on, and once I tested the impact it wasn't good.

elazutkin-dynata · 2020-02-07T21:53:48Z

The patch is about a choice. Nobody forces people to use deflate, gzip, or brotli. If a certain compression makes sense for your particular setup (cache or not, or CloudFront, or any other distribution technologies) — it is there with no extra dependencies.

indeyets · 2020-02-08T08:07:56Z

Brotli is much slower than gz if you want better compression, and even if you relax the compression level, still slower than higher level gz for less compression

@tracker1 it is not. You probably think about zopfli. Brotli is faster than gzip on moderate compression levels still giving better results

see https://engineering.linkedin.com/blog/2017/05/boosting-site-speed-using-brotli-compression fort example (my tests show the same)

Another excessive test: https://community.centminmod.com/threads/round-3-compression-comparison-benchmarks-zstd-vs-brotli-vs-pigz-vs-bzip2-vs-xz-etc.17259/

jonathanong · 2020-04-16T04:01:08Z

index.js

@@ -29,7 +32,7 @@ const encodingMethods = {
 */

 module.exports = (options = {}) => {
-  let { filter = compressible, threshold = 1024 } = options
+  let { brotliOptions, filter = compressible, threshold = 1024 } = options


i'd prefer if it were just options.brotli but i will leave it

I am totally open to change it. Please think about it and if that's your preference I will rename.

Personally I don't like brotliOptions either — too long.

jonathanong · 2020-04-16T04:01:34Z

index.js

-    const encoding = ctx.acceptsEncodings('gzip', 'deflate', 'identity')
-    if (!encoding) ctx.throw(406, 'supported encodings: gzip, deflate, identity')
+    let encoding = null
+    if (brotliSupport && brotliOptions !== null) encoding = ctx.acceptsEncodings('br') // 1st choice


why is this first choice? shouldn't we honor what the client requests?

i.e. we should be doing like...

const acceptedEncodings = ['gzip', 'deflate', 'identity'] if (brotliSupport && brotliOptions !== null) acceptedEncodings.unshift('br') const encoding = ctx.acceptsEncodings(...acceptedEncodings)

this is my only concern as this is implementation is technically against specifications

Sounds good. I will update the PR with:

brotliOptions ⇒ brotli

Rely on acceptsEncodings() for selecting the encoding.

Please let me know, if you change your mind on number 1 above.

A sidenote: when I was working on the patch I looked at #80 and it used a similar mechanism for other compressors: options.gzip, options.deflate. Your proposal options.brotli follows the same pattern. Do you want me to add a support for options.XXX, where XXX is a compressor name as defined in the standard? It could be a separate PR, if that's what you prefer.

i guess for your sidenote, we should just stick with either br or brotli (not both) in our code except for acceptsEncodings, right?

Yeah, why not.

* `brotliOptions` => `br` * `options.XXX` style for custom options, where XXX is an encoding: br, gzip, or deflate. * One test always fails because brotli is never selected. * Obviously I can force it for the test, but... * No PR is updated yet pending the decision on how to proceed.

uhop · 2020-04-16T05:27:18Z

I updated my code as we discussed above, one test has failed, and now I remember why I did this two-step br selection process: for some reason acceptsEncodings() always selects the first one in the list of accepted encodings, and all browsers send accept-encoding like that: gzip, deflate, br for compatibility reasons. It means there is no way to send brotli-encoded content to browsers ever, which defies the purpose of the patch. Only with custom clients.

The standard defines accept-encoding like a negotiation:

Client sends a list of encodings, which it can understand.
- The list is presented by accept-encoding request header.
- If the header is missing, any encoding is fine.
Server can select any encoding it likes and sends a payload encoded with it.
- The selected encoding is indicated by content-encoding response header.
That's it. There is no third step.

Does the order imply the client's preference? No. In order to do that the standard defines an ability to add weights using q=0.9 notation, which serves as a hint to a server, but does not force the selection.

To wit:

The standard: https://tools.ietf.org/html/rfc7231#section-5.3.4
MDN explains negotiations: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding

My patch gives an edge to br checking it first. No standard violation there. If it is not selected, other encodings will be tried. What if I don't want to use brotli like ever? The original patch allows assigning null to brotliOptions, which indicates that brotli is prohibited. I thought it is flexible enough to control how user wants to handle it.

I considered other ways to specify my preference with parameters:

Add a list of compressions I want to accept in the order of preference.
- With some reasonable default like gzip, deflate, identity if you don't want brotli by default.
Use brotli only if brotliOptions (soon to be shortened) are present.
- So it is always off by default.
- If I want to turn it on I specify something like {brotliOptions: {}}.

I decided against this approach and decided to handle it differently (see my patch).

Obviously this problem is beyond simple engineering — we can do it this way or that way. What is your position on that?

I prepared the new code as discussed. I hold off on it until the decision is made. You can look at it here: uhop@8f073b2

PS: The name brotliOptions was taken from Node's documentation: Options vs. BrotliOptions.

uhop · 2020-04-16T05:43:59Z

Aside:

I wish acceptsEncodings() used the order of its array argument to indicate the preference. Instead, it uses the client's order, which is wrong and gives no say to a server — the only active party in the negotiations, which is in the right position to make a decision.

uhop · 2020-04-16T06:00:11Z

Heh, I forgot that the PR is automatically updated. So you can look at changes here as well.

jonathanong · 2020-04-16T18:14:36Z

i believe acceptsEncodings() does take order of arguments as preference as long as the client's preferences all have equal weight (which is usually the case as far as I know)

uhop · 2020-04-16T18:29:56Z

Look at https://github.com/koajs/compress/pull/92/files#diff-518498ef0535a2e4c194b95d1cff9c3fR341 — this test now fails. It shouldn't.

How to repro:

Clone my repository.
Run npm test.

It will execute ctx.acceptsEncodings(['br', 'gzip', 'deflate', 'identity']), which will return "gzip". In the test you will see that I send accept-encoding as gzip, deflate, br string.

What am I doing wrong?

It almost looks like I need to test for an encoding like that (meta code):

const acceptedEncodings = []
// collect encodings, example:
if (brotliIsAvailable && options.br !== null) acceptedEncodings.push('br')
// ...
// now we have something like: ['br', 'gzip', 'deflate', identity']
let encoding = null
for (const encodingName of acceptedEncodings) {
  encoding = ctx.acceptsEncodings(encodingName)
  if (encoding) break
}
// now we have it

jonathanong · 2020-04-17T00:38:40Z

I'm guessing you are "pushing" after acceptedEncodings = ['gzip', 'deflate', identity'] try it with:

acceptedEncodings.unshift('br')

br should be first

jonathanong · 2020-04-17T00:41:46Z

in repl in this repo:

> Negotiator = require('negotiator')
[Function: Negotiator] { Negotiator: [Circular] }
> x = new Negotiator({ headers: {'accept-encoding': 'gzip, deflate, br'}})
Negotiator {
  request: { headers: { 'accept-encoding': 'gzip, deflate, br' } }
}
> x.encodings(['br', 'gzip'])
[ 'gzip', 'br' ]
> x = new Negotiator({ headers: {'accept-encoding': 'br, gzip, deflate'}})
Negotiator {
  request: { headers: { 'accept-encoding': 'br, gzip, deflate' } }
}
> x.encodings(['br', 'gzip'])
[ 'br', 'gzip' ]

jonathanong · 2020-04-17T00:43:25Z

o i c negotiator doesn't prioritize the output based on input if they all have the same weights. i'd rather do that, but let me see if that's according to spec

jonathanong · 2020-04-17T00:50:42Z

another issue is that if the client specifies *, we should not use br until all clients support it. at this point, we should do our own content negotiation

uhop · 2020-04-17T01:57:12Z

I'm guessing you are "pushing" after acceptedEncodings = ['gzip', 'deflate', identity'] try it with: acceptedEncodings.unshift('br'). br should be first.

No. I do exactly what you suggested: https://github.com/koajs/compress/pull/92/files#diff-168726dbe96b3ce427e7fedce31bb0bcR56

uhop · 2020-04-17T02:00:09Z

in repl in this repo:

You can see that Negotiator ignores the order of parameters and always returns in the order of accept-encoding. That's basically exactly what I see and described above.

uhop · 2020-04-17T02:08:12Z

another issue is that if the client specifies *, we should not use br until all clients support it.

What does it mean? If tomorrow everybody and their dog decided to support br, somebody would go out and write a custom client that doesn't support it.

I started to pay attention to koa-compress precisely because I wanted it for HTTP requests from Node. I wrote a custom client, which does that, and decided to add br to both ends.

at this point, we should do our own content negotiation

How can we negotiate with *? Or for that matter with a static list of accepted encodings?

Personally I don't care for *:

I never saw it in the wild.
If I write a client I will avoid *
Because if someone specified * the behavior is indeterministic.

Fixing ESLint gripes in a test function.

f7cf2f7

jonathanong reviewed Apr 16, 2020

View reviewed changes

jonathanong mentioned this pull request Apr 16, 2020

Add brotli support #80

Closed

jonathanong self-assigned this Apr 16, 2020

No need for spread, the method takes an array as well.

ad8a70e

jonathanong mentioned this pull request Apr 18, 2020

brotli support #99

Merged

4 tasks

jonathanong closed this in #99 Apr 28, 2020

Added native Brotli support. #92

Added native Brotli support. #92

Uh oh!

Conversation

uhop commented Nov 4, 2019

Uh oh!

uhop commented Nov 4, 2019

Uh oh!

codecov bot commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

uhop commented Nov 4, 2019

Uh oh!

markulrich commented Feb 6, 2020

Uh oh!

tracker1 commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elazutkin-dynata commented Feb 7, 2020

Uh oh!

indeyets commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanong Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

uhop Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanong Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanong Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanong Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

uhop Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanong Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhop Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

uhop commented Apr 16, 2020

Uh oh!

uhop commented Apr 16, 2020

Uh oh!

uhop commented Apr 16, 2020

Uh oh!

jonathanong commented Apr 16, 2020

Uh oh!

uhop commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanong commented Apr 17, 2020

Uh oh!

jonathanong commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonathanong commented Apr 17, 2020

Uh oh!

jonathanong commented Apr 17, 2020

Uh oh!

uhop commented Apr 17, 2020

Uh oh!

uhop commented Apr 17, 2020

Uh oh!

uhop commented Apr 17, 2020

Uh oh!

Uh oh!

codecov bot commented Nov 4, 2019 •

edited

Loading

tracker1 commented Feb 7, 2020 •

edited

Loading

indeyets commented Feb 8, 2020 •

edited

Loading

uhop Apr 16, 2020 •

edited

Loading

jonathanong Apr 16, 2020 •

edited

Loading

uhop commented Apr 16, 2020 •

edited

Loading

jonathanong commented Apr 17, 2020 •

edited

Loading