Added support for brotli ('br') content-encoding

[danielgindi]

expressjs/body-parser#403

https://medium.com/oyotech/how-brotli-compression-gave-us-37-latency-improvement-14d41e50fee4
https://caniuse.com/#feat=brotli

[dougwilson]

Make sure to update the documentation as well around it supporting br.

[danielgindi]

@dougwilson There's an issue with object-assign polyfill - it does not support the very outdated versions of node.js

[dougwilson]

There's an issue with object-assign polyfill - it does not support the very outdated versions of node.js

I guess just pick a different module or an older version of that module.

[dougwilson]

I added a comment about the faster statement and still have an open question on how a user can change compression level of br.

[dougwilson]

I added a comment about the faster statement and still have an open question on how a user can change compression level of br. I'm also having trouble to actually get br compression to even work at all with Chrome. I'm trying to figure it out, as our number 1 issue opened here is how to get this module working, so having concrete information for how to get br working with a web browser (Chrome, for instance) would help a lot. For reference I used the example in the README and your branch and Chrome continues to only show it using gzip, even when the connection is https (which my understanding is a requirement for br to work in Chrome).

[danielgindi]

I added a comment about the faster statement and still have an open question on how a user can change compression level of br. I'm also having trouble to actually get br compression to even work at all with Chrome. I'm trying to figure it out, as our number 1 issue opened here is how to get this module working, so having concrete information for how to get br working with a web browser (Chrome, for instance) would help a lot. For reference I used the example in the README and your branch and Chrome continues to only show it using gzip, even when the connection is https (which my understanding is a requirement for br to work in Chrome).

Look at Chrome's Accept-Encoding: gzip, deflate, br - it puts br last. I'm guessing they put it last as a transition period, as some poor servers out there fail when brotli is specified, or intermediaries doing even worse.

Or maybe it's because when it first arrived they considered it a good compression for WOFF fonts, but they always tested with the highest compression levels. Today people know that with level 4 you have better results on all kinds of files.

[dougwilson]

Hi @nicksrandall thank you so much! I am still on mobile, and it looks like there is now a substantial amount of code to review 😭 . I'm a bit concerned about why we should be adding such a large amount of code directly to this library, vs adding whatever is needed into Express.js's own accepts or other library? It also seems to be in violation of their software license, as it is licensed under MIT, but you did not include the given MIT license and copyright with this substantial part of the software here...

I think (but haven't yet tried) based on the code that it is not as standards compliant as the previous module used here, and we will need to go over this new implementation and compare to the standards with a fine-toothed comb. It seems to me that, for instance, it is not comparing the content coding as a case-insensitive manor as specified in RFC 7231 (but since I'm mobile I haven't tested that -- I am just looking at the code on a small screen 😅 ).

If we want to move forward with this implementation, please give me until the next couple weeks to have the time to circle around to review all of this code... the first few looks I've noticed multiple spec violations, even after you made a bunch of changes, so I'm not sure at what level of detail this large replacement code has been looked over to feel comfortable sending to the 8M+ daily downloads, as we want to really be sure we are not regressing behavior here 🙆‍♂️ .

[nicksrandall]

I didn't want to loose what was happening here so I have created an alternate PR (#173) that provides what I believe to be the absolute minimal code change to this library to support brolti in a backwards compatible way.

I'm just hoping that one of these PRs gets merged in the near term. Please let me know which direction you think would be best to continue to pursue.

[danielgindi]

@dougwilson I've removed the dependency on koa's modified code, and implemented a simple parsing function for the encoding, with built-in preferred-encoding support. this results in less code, and a faster code.

Passing all old tests, new tests, and the latest ones added by @nicksrandall

I really don't see anything blocking a merge now, and it would be great to have this in production :-)

---

I've also rebased on master, as apparently my master was not in sync with yours.

[dougwilson]

I'm a bit concerned about why we should be adding such a large amount of code directly to this library, vs adding whatever is needed into Express.js's own accepts or other library? We target every library to only have a single index.js file if possible, and since we already split out this functionality long ago into these other modules, it seems like putting that functionality there is the correct path forward, rather than putting a large about of code in here that doesn't have a test suite around it.

Besides that, even just putting that code in here seems to have licensing issues, as it seems to be a large chunk of the negotiator module, which is licensed under MIT, which requires the original copyright to be kept with substantial portions like the file added here. Just making the changes direct in our dependency (which is part of Express.js) avoids that issues altogether.

[danielgindi]

I'm a bit concerned about why we should be adding such a large amount of code directly to this library, vs adding whatever is needed into Express.js's own accepts or other library? We target every library to only have a single index.js file if possible, and since we already split out this functionality long ago into these other modules, it seems like putting that functionality there is the correct path forward, rather than putting a large about of code in here that doesn't have a test suite around it.

Besides that, even just putting that code in here seems to have licensing issues, as it seems to be a large chunk of the negotiator module, which is licensed under MIT, which requires the original copyright to be kept with substantial portions like the file added here. Just making the changes direct in our dependency (which is part of Express.js) avoids that issues altogether.

This module uses accepts which is not part of the expressjs namespace, I do not know who is in charge of that module, but I took a look at that and it seems like it's jumping through hoops for the sake of keeping it generic, and adding a "preferred" functionality will probably not be accepted well there (even though the module is called "accepts"...).
Note that originally there was a "preferred" functionality implemented here, in compression, and not in accepts. Although it was only for one "preferred", but still.

I don't see a "large chunk" of negotiator module, except for a very small for-loop that extracts a single value from = pairs. There are not a lot of ways you could write that piece of code, and I really do not believe that this is an issue in any way.

Do you want to get compression to move forward and support modern compressions or not?

[dougwilson]

This module uses accepts which is not part of the expressjs namespace, I do not know who is in charge of that module

It is part of Express.js: http://expressjs.com/en/resources/community.html#express-is-made-of-many-modules

and adding a "preferred" functionality will probably not be accepted well there

Why not? I mean, just look at the person who is committing there... it is me. Are you saying I would not accept what I am suggesting you to put there?

I don't see a "large chunk" of negotiator module, except for a very small for-loop that extracts a single value from = pairs. There are not a lot of ways you could write that piece of code, and I really do not believe that this is an issue in any way.

Sure, you may have unrolled some of those functions together, but unless you are saying you definately didn't copy and paste from negotiator and just move around the code, then it is certainly the original license at play on that code.

[danielgindi]

Why not? I mean, just look at the person who is committing there... it is me. Are you saying I would not accept what I am suggesting you to put there?

Good to know 😂

Sure, you may have unrolled some of those functions together, but unless you are saying you definately didn't copy and paste from negotiator and just move around the code, then it is certainly the original license at play on that code.
What happened is actually more simple: I took a look at the regex, did not really trust it as people tend to let regexes run free. So I wrote one carefully on http://regexpal.com/, and grouped it the same way. Then I copied specifically the param split part. All the other code was written by me in a few minutes.

I do not really get the insistency on splitting everything to a million pieces, I don't see a value to any extremity, but on the other hand I don't mind making a PR to negotiator and accepts, as long as there's a willingness to move forward.

[danielgindi]

@dougwilson If we're at it - why did you filter out q=0 in negotiator? These are valid quality values, just prioritized the lowest.

[dougwilson]

@dougwilson If we're at it - why did you filter out q=0 in negotiator? These are valid quality values, just prioritized the lowest.

This is because when you combine the module's API and spec, that is the correct functionality. The API of the module only returns what is acceptable. The spec says that 0 is a special value of not acceptable (vs just the lowest acceptability, which would be 0.001). You can find the spec in RFC 7231.

If the representation's content-coding is one of the content-codings listed in the Accept-Encoding field, then it is acceptable unless it is accompanied by a qvalue of 0. (As defined in Section 5.3.1, a qvalue of 0 means "not acceptable".)

The weight is normalized to a real number in the range 0 through 1, where 0.001 is the least preferred and 1 is the most preferred; a value of 0 means "not acceptable". If no "q" parameter is present, the default weight is 1.

[danielgindi]

@dougwilson you have a PR pending

[danielgindi]

I guess everyone are in vacation now

[nicksrandall]

@danielgindi just want to say that this is good work. Thanks for pushing for this.

[danielgindi]

At this point I'm convinced this is not going to happen.

[Kle0s]

At this point I'm convinced this is not going to happen.

Would you like to create an npm package from your fork? Or should I? I really think it's much needed (and would like to use it myself)

[nicksrandall]

If it is helpful, I’ve built this package and am using it in production in several places: https://github.com/nicksrandall/compression

[maggie44]

If it is helpful, I’ve built this package and am using it in production in several places: https://github.com/nicksrandall/compression

Tempting, thanks for this. How has it been running, any issues?

And this one that seems pretty active: https://github.com/Econify/compression-next#readme

[vinayakkulkarni]

LGTM

[nithin-murali-arch]

Looks like the PR is approved, could you folks resolve the conflicts and merge? Is there anything else we're waiting on?

[dgautsch]

Any plans on having this merged or is https://www.npmjs.com/package/shrink-ray-current the current best solution?