Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change resolved Accept-Encoding to honour server side order preferece #49

Closed
wants to merge 1 commit into from

Conversation

gmokki
Copy link

@gmokki gmokki commented Jul 23, 2016

...when comparing client provided encodings with equal quality
Also change the quality of automatically added identity encoding to be lower than other encodings instead of equal to lowest seen quality.

This change is required by pillarjs/send#108 to be able to use the negotiator.

@dougwilson dougwilson self-assigned this Aug 14, 2016
@dougwilson dougwilson added the pr label Aug 14, 2016
@dougwilson
Copy link
Contributor

Thank you, @gmokki! Sorry I have been away for a while :( Looking at the change, it doesn't seem like a backwards-compatible change, does it? Mainly, I'm concerned because Express 4.x directly exposes the results of this module, and just do not have the bandwidth to maintain two version lines.

  1. Would it be possible to place the behavior change behind an opt-in flag?
  2. This seems like a change that should uniformly be applied across all the different negotiations. Can that be added to maintain a uniform API?

@gmokki
Copy link
Author

gmokki commented Aug 19, 2016

You are correct that the change is not backwards compatible. I can put the different sorting order behind an option.

However, I'd like to argue that because current top browsers:

  • put "gzip" as the first accept-encoding
  • do not use quality levels

Then any user of negotiator.accept(['anything', 'gzip', 'other']) will have always selected gzip. And basically all the other options have been always been ignored.
Because the selection has never worked in real life I hope the accept with array argument has never actually been used in real projects and thus breaking backwards compatibility should not break anything badly.

But again, I'll try to add a commit with configurable default so that you can choose take it if you prefer avoiding backwards incompatible changes.

@dougwilson
Copy link
Contributor

It doesn't matter what browsers do, as there is no reason why this module would not be used in REST APIs getting requests from non-browser clients.

It is a breaking change, regardless of speculation.

@federomero
Copy link
Contributor

Hi, I just want to point out that I think this change breaks compliance with RFC 2616.

I couldn't find anywhere in the RFC that the order of the encodings indicates any kind of preference from the client. But it's explicitly stated in the RFC that:

If more than one media range applies to a given type, the most specific reference has precedence

Even though that sentence refers to the Accept header, I think the same principle applies to Accept-Encoding.

I don't mind honouring client order, but I wouldn't do it at the cost honouring specificity.

@gmokki gmokki force-pushed the master branch 5 times, most recently from 32ecd8f to 2ac5a24 Compare August 26, 2016 14:08
@gmokki
Copy link
Author

gmokki commented Aug 26, 2016

I have now rewritten the patch so that the sorting algorithm can be chosen by passing suitable options object. The current algorithm is maintained as the default.

Do I need to add similar functionality also to the other parsers or do you think adding options only to encoding parser is acceptable?

Options:
client: use client quality levels and client order for equal quality levels
clientThenServer: use client quality levels and server order for equal quality levels
server: use server order

The 'client' preferred order is the default to match with previous implementation.

Signed-off-by: Jonas Berlin <jonas.berlin@nitorcreations.com>
gmokki pushed a commit to NitorCreations/digitransit-ui that referenced this pull request Nov 23, 2016
…the browsers.

- optimize png images with pngquant (quality 95) and advpng
- optimize svg images with svgo
- create precompressed variants of js, css, svg files: gz using zopfli and br using brotli

Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through:
- jshttp/negotiator#49
- pillarjs/send#108
gmokki pushed a commit to NitorCreations/digitransit-ui that referenced this pull request Nov 23, 2016
…the browsers.

- optimize png images with pngquant (quality 95) and advpng
- optimize svg images with svgo
- create precompressed variants of js, css, svg files: gz using zopfli and br using brotli

Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through:
- jshttp/negotiator#49
- pillarjs/send#108
gmokki pushed a commit to NitorCreations/digitransit-ui that referenced this pull request Nov 25, 2016
…the browsers.

- create precompressed variants of js, css, svg files: gz using zopfli and br using brotli

Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through:
- jshttp/negotiator#49
- pillarjs/send#108
gmokki pushed a commit to NitorCreations/digitransit-ui that referenced this pull request Nov 25, 2016
…the browsers.

- create precompressed variants of js, css, svg files: gz using zopfli and br using brotli

Currently using forked versions of expressjs/send/negotiator while waiting for the PRs to go through:
- jshttp/negotiator#49
- pillarjs/send#108
@ephys

This comment has been minimized.

@yvele

This comment has been minimized.

@wesleytodd
Copy link
Member

This has been sitting for a very long time and looked to be more contentious than I was willing to take on after so long. I landed #59 which appears to cover some of the need here. If someone wants to re-open a smaller change I would be happy to re-assess, but I am going to close this one for now.

@wesleytodd wesleytodd closed this Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants