jshttp / negotiator

An HTTP content negotiator for Node.js
MIT License
309 stars 33 forks source link

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

Closed gmokki closed 1 week ago

gmokki commented 8 years ago

...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 https://github.com/pillarjs/send/pull/108 to be able to use the negotiator.

dougwilson commented 8 years ago

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 commented 8 years ago

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:

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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?

ephys commented 7 years ago

Any update on this? Support for brotli grows but this prevents us from using it plus gzip as a fallback because gzip is always returned by preferredEncodings.

yvele commented 5 years ago

Same problem here, with accept-encoding: gzip, deflate, br (basically used by many Chromes) and server preference ["br", "gzip", "identity"] I'm getting gzip instead of br 🤔

The client with gzip, deflate, br hasn't expressed any preference, so the server preference should be used

Edit: hapijs/accept actually works as expected.

wesleytodd commented 1 week ago

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.