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

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.

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.

I will make a change so that if brotli and gzip are preferred equally (regardless of order) the system will prefer brotli. Otherwise, the system will prefer the more preferred compression algorithm. Sound reasonable?

My thinking is to not reinvent the wheel so I've pulled in koa-compress' logic to determine which content encoding header is preferred as it conforms the rules we've specified above. I've also added a test case to verify.

My only concern here is node version support. What is the official node version support for this lib?

My only concern here is node version support. What is the official node version support for this lib?

The official Express.js middlewares support the same Node.js versions as the current version of Express.js. The Travis CI has the supported versions listed in the test matrix.

I will make a change so that if brotli and gzip are preferred equally (regardless of order) the system will prefer brotli. Otherwise, the system will prefer the more preferred compression algorithm. Sound reasonable?

Yea, that sounds fine to me, as it seems like that is what the web browsers are actually doing for this case.

The official Express.js middlewares support the same Node.js versions as the current version of Express.js.

Forgive me as I'm not super familiar with Travis CI but according to https://expressjs.com/en/changelog/4x.html Express.js seems to only support node 10.x and 12.x but the test matrix goes all the way down to 0.8? Did I interpret something?

My tests are passing with Node > 6.x. Is that sufficient?

I think you are mistaking the change log for adding support to Node.js versions as supporting only those versions. It supports the same versions the Travis CI matrix of this module supports, so the CI just needs to pass with the existing matrix unless the idea is to hold this PR until the next major version instead of releasing as a semver minor, if that is the desire. Express 5 will be released soon, so it would probably only need to wait a few months if that is what path you want.

Though it seems like this change has broken the accept in other ways. For example, given the header identity;q=0 seems to throw an exception instead of sending back the uncompressed response. Also the behavior for the header gzip;q=0.001 seems to be to no longer compress instead of sending back gzip.

Ok, I have all the tests passing on all versions of node, including a new test that tests the "gzip;q=0.001" value.

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 🙆‍♂️ .

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.

@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.

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.

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?

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.

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.

@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 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.

@dougwilson you have a PR pending

I guess everyone are in vacation now