Use identity as default value for Accept-Encoding if it is blank or not provided #110
Conversation
| @@ -61,7 +61,7 @@ module.exports = (options = {}) => { | |||
| const encodings = new Encodings({ | |||
| preferredEncodings | |||
| }) | |||
| encodings.parseAcceptEncoding(ctx.request.headers['accept-encoding'] || undefined) | |||
| encodings.parseAcceptEncoding(ctx.request.headers['accept-encoding'] || 'identity') | |||
There was a problem hiding this comment.
the default is not actually identity but *. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#Directives
*Matches any content encoding not already listed in the header. This is the default value if the header is not present. It doesn't mean that any algorithm is supported; merely that no preference is expressed.
There was a problem hiding this comment.
i'm happy to make an option to set a default value though
There was a problem hiding this comment.
I found it really confusing when I used curl to hit my server and got back compressed responses even though I didn't ask for them.
Even if the spec says that the default is "don't care" I think on the server side it's better to play it safe and only use compression formats explicitly supported by the client.
I think HTTP clients that support compression will almost always send the header, and ones that do not support compression will probably almost never send it, so it makes more sense to me that way (assume identity).
Anyway, I've made my case :) Do with it what you will.
There was a problem hiding this comment.
i agree, that spec is confusing
There was a problem hiding this comment.
FWIW, this does seem to be a different behavior than was present in v3 of this library. Since the previous version of the library used koa's ctx.acceptsEncodings, it means this behavior differs from koa (and for that matter from express). So I would assume the confusion largely comes from that.
The previous version ultimately used negotiator, which always adds identity to the list of accepted encodings: https://github.com/jshttp/negotiator/blob/master/lib/encoding.js#L46-L56
If the new behavior is more correct according to the spec, maybe its worth explicitly calling that out since its a departure from previous versions?
|
Since this might be the wrong solution, I filed an issue to track this: Then any new solution can refer back to that issue. |
|
My 2c: the spec is confusing. From my experience, all browsers send A sidenote:
I suggest to implement two things:
I think it breaks nothing and satisfies expectations of most users. If this is acceptable to @jonathanong and other participants I can update the PR or create a new one with the fix. |
|
Is there anything I can do to help this along? This change broke some APIs for us and we had to downgrade in a hurry (the use-case in that situation was not browser to Node, it was .NET Framework APIs calling a Node microservice). |
|
Can you please update the PR per your notes @uhop ? I just reproduced same issue on curl. |
|
v5.0.0 published to npm and GitHub |
I noticed that the existing test for "no Accept-Encoding" header was not valid because the library used to test was adding a default value for it. The test in this PR fails because of that.
I have a PR on
superagentto allow unsetting theAccept-Encodingheader, I suppose once that is fixed this PR could be updated.Or there might be another hack to make the test work properly.