feat: Adding support for brolti #173
Conversation
nicksrandall
force-pushed
the
nick/brotli
branch
9 times, most recently
from
ceb509a
to
688b0d4
Compare
|
@dougwilson this is ready for review... I think you'll like this PR much better |
nicksrandall
force-pushed
the
nick/brotli
branch
6 times, most recently
from
1c28fc3
to
df37bab
Compare
nicksrandall
force-pushed
the
nick/brotli
branch
from
df37bab
to
dc8e920
Compare
|
@dougwilson what do you think of the direction in this PR? Anything I can do to help you with the QA/Approval process? |
|
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 😂 |
|
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? |
|
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 |
|
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. |
There was a problem hiding this comment.
I am on my phone, but tried to do review what I could to help move it along. It is not comprehensive and I didn't actually run anything, so please forgive me, but I thought that I could at least use my time here waiting in the dr. office to do something for you since you pinged me :)
index.js
Outdated
| method = accept.encoding(['gzip', 'identity']) | ||
| } | ||
| // compression method | ||
| var accept = accepts(objectAssign({}, res, { headers: headers })) |
There was a problem hiding this comment.
AFAIK the accepts API expects the request object, not a response object clone.
There was a problem hiding this comment.
Good catch, this is a bug (although it didn't actually effect anything because accepts library only reads the headers). This has been fixed.
There was a problem hiding this comment.
I'm not sure if this is even cloning the res correctly, anyhow; it seems to only copy over the own, enumerable properties; all the methods from the prototype and such are lost, for instance.
There was a problem hiding this comment.
I've updated this code so that we don't pass in the entire (cloned) request and instead with just pass in the cloned headers. Thats all the accepts function reads from anyway.
There was a problem hiding this comment.
So that of course does end up violating the API contract of that dependency. We should either hard-pin that dependency so that we have to manually bump the version for any changes since this would now be relying on the internal behavior of that dependency to not access anything in outside of that one property, or perhaps get the dependency to change/clarify the contract to only being the headers. If we do go the pin dependency route, we'll want to check that if it then passes the req object to a dependency of it's own, that that is pinned within that module as well. This will prevent the possibility of sudden breakage when dependencies, dependencies of dependencies, etc. update
| * | ||
| * @private | ||
| */ | ||
| function prioritize (str) { |
There was a problem hiding this comment.
Looking at this, it seems like this can be an option added to the accept negotiation logic directly? Like an option to prefer server order over client order? I think that is effectively what this is doing, or am I misunderstanding?
There was a problem hiding this comment.
Great idea. I have added a priorizeClient option that will allow users to opt-out of this functionality.
For what it's worth, we were already using this behavior because we were prioritizing GZIP over deflate even when deflate came first. As such, I think "opt-out" vs "opt-in" is acceptable in this case.
There was a problem hiding this comment.
I'm sorry, I think I didn't explain very well my thought on this; it wasn't to add a parameter here to this library's API, but rather to the accepts API. As far as adding this as a parameter to this module's API, I'm not sure if it's actually necessary, though if it is going to be surfaced as a switch, I would kind of expect such a switch to default to the spec compliant method. But again, I'm not sure if this actually needs to be a switch for the user; a better user switch would actually be to turn on/off each individual compression method, but that would be out of scope for this PR.
There was a problem hiding this comment.
I see what you are saying but we were already out of spec because we were prioritizing gzip over deflate even when deflate came first. The option to be fully spec compliant would be a new feature for this library.
I also will argue that most people do not want the spec compliant behavior and they will prefer the default option that we have here.
There was a problem hiding this comment.
I think you are miss understanding what I am saying on this comment again, I'm really sorry. I must not be good at communicating my idea here through writing and that is my bad. I tried twice and it wasn't successful :( Perhaps if you desiure to move this forward would you be willing to do some kind of voice call?
index.js
Outdated
| * @private | ||
| */ | ||
| function sortEncodings (a, b) { | ||
| if (a.indexOf('br') >= 0) { |
There was a problem hiding this comment.
The encoding are case insensitive according to the spec, but this us treating them as case sensitive.
There was a problem hiding this comment.
Another great catch, I now lowercase the value of the header before I look for any specific values.
There was a problem hiding this comment.
This seems to now violate the spec in a different way: the q parameter name is case-sensitive, so br;q=0 header would have br with a quality of 0, but br;Q=0 would have a quality of 1. This change you made will now accidentally treat the latter as the former.
There was a problem hiding this comment.
I see what you are saying. I've updated the code so that this sorting function will sort the items in the header but it will not change their original case. It will also work with any casing of encoding values.
There was a problem hiding this comment.
Is there a reason why it is using .indexOf instead of just a comparison? I would think .indexOf would identify bro as br or similar. I'm not sure what that would all entail. I glanced again at the spec and this header doesn't allow random parameters to be specified, so it does seem unlikely to get mixed up in that way, but when looking, I realize that this header does accept the special * value. I know this sorting does not take that * into account there, so I wonder if that will cause issues with the logic? I tried a few different headers to see what the outcomes are and was curious if these make sense:
* : Results in br encoding
*, gzip: Results in gzip encoding
gzip, *: Results in gzip encoding
nicksrandall
force-pushed
the
nick/brotli
branch
from
dc8e920
to
6447efa
Compare
nicksrandall
force-pushed
the
nick/brotli
branch
from
6447efa
to
3b31349
Compare
|
@nicksrandall Thanks for this change, is there a plan for closing this. We are planning to go for production. |
Co-authored-by: Matthias Le Brun <mlbli@me.com>
|
Is there any plan for close this PR? I'm waiting for this feature to release |
|
I think that #172 is more likely to land. I'll close this PR as soon as that PR lands. |
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, brinstead ofbr, 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, brwe will usebr(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.