feat: Adding support for brolti

nicksrandall commented 4 years ago

Fixes #71

TL;DR

There has been several attempts (#172, #158) to add brotli compression support to this library and so far none have been successfully merged (for various reasons). This PR is yet another attempt to support brotli compression in a backwards compatible way while also reducing existing code change as much as possible to ensure stability of the library.

What is the problem that is trying to be solved?

The brotli compression algorithm is generally more efficient than gzip and now supported in recent versions of Node.js and in all major browsers. Http clients (like web browsers) can specify an "Accept-Encoding" header in their requests to share which compression algorithms they support and prefer. According to the spec, when two values have the same preference, the first value will be used (seems reasonable right?).

However, many browsers (including Google Chrome) send the "br" (brotli) value last even though it is generally preferred to the other algorithms (ie they send gzip, deflate, br instead of br, gzip, deflate). This causes the brotli compression algorithm to be de-prioritized and unused.

What can we do about it?

We can follow a well-established convention to deviate from the spec slightly and force the server to choose the "preferred" compression algorithm when the client has (basically) stated that it doesn't not explicitly prefer one algorithm over another.

So, for example, if we get an "Accept-Encoding" header value of gzip, deflate, br we will use br (brotli) because brotli is more efficient over gzip and their preference (set by client) is both 1 (the default value).

However, if we get gzip;q=0.8, br;q=0.1 (q is level of preference from 0 to 1 where 1 is most preferred) we will use 'gzip' because the client has explicitly stated that it is more preferred than brotli.

nicksrandall commented 4 years ago

@dougwilson this is ready for review... I think you'll like this PR much better

nicksrandall commented 4 years ago

@dougwilson what do you think of the direction in this PR? Anything I can do to help you with the QA/Approval process?

dougwilson commented 4 years ago

Hi @nicksrandall apologies if I didn't mention it before: sorry I haven't been able to take a look just yet; I am currently spending all my allocated OS time right now addressing some security issues that have been reported. Please give me about a week to get everything settled and I will be taking a look, thank you 🙏 -- I've noticed you have made about 6+ pushes to this branch even after you noted it was ready for review; it seems like it is still having some edge cases sorted out anyhow 😂

nicksrandall commented 4 years ago

Hi @dougwilson not trying to be pushy, just checking in to see where we are at with this PR? Any change you'll have some time to review anytime soon?

nicksrandall commented 4 years ago

The only potential issue I can see with this change is that I'm cloning the request object on every request before I sent it to the accepts library to determine which encoding method should be used. As far as I know, the accepts library only uses the headers property on the request object so I could only send the cloned (and altered) headers to the accepts library with the rest of the request object being omitted. Therefore I would have less to clone so that might be a bit more performant but it could break something if accepts library decides to use other parts of the request in the future.

dougwilson commented 4 years ago

The security issue is still on going at this time, I apologize. Github is the reporter and I have been working with their security group still on the situation.