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

Vary: Accept-Encoding missing in zlib examples #25495

Closed
nigoroll opened this issue Jan 14, 2019 · 5 comments
Closed

Vary: Accept-Encoding missing in zlib examples #25495

nigoroll opened this issue Jan 14, 2019 · 5 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. zlib Issues and PRs related to the zlib subsystem.

Comments

@nigoroll
Copy link

Ref: https://github.com/nodejs/node/blob/master/doc/api/zlib.md#compressing-http-requests-and-responses

It appears to me that the examples lack setting the Vary: Accept-Encoding Response header.

@Fishrock123 Fishrock123 added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. good first issue Issues that are suitable for first-time contributors. doc Issues and PRs related to the documentations. zlib Issues and PRs related to the zlib subsystem. labels Jan 14, 2019
@DamianRivas
Copy link

DamianRivas commented Jan 14, 2019

@Fishrock123 Hi! I'd like to help out here. I'm afraid I can't tell what the problem is exactly. Would the change be something like this?

http.createServer((request, response) => {
  ...
  response.setHeader('Vary: Accept-Encoding');
  ...

EDIT: Oops, that's the client.

@addaleax
Copy link
Member

@Fishrock123 @nigoroll Are you sure that this should happen? It’s not central to the point of the documentation, as it introduces the much bigger topic of HTTP caching into our compression docs.

@DamianRivas I think this only makes sense for the server example, since Vary is a HTTP server header.

@nigoroll
Copy link
Author

nigoroll commented Jan 16, 2019

@addaleax It is plain wrong to use the value of Accept-Encoding to determine the Content-Encoding and not set Vary, unless any other downstream caching is prevented, and even then, for good interop with caches, I'd advise to still set it correctly, because cache admins may override cache directives.

I am not sure what exactly you mean by It’s not central to the point of the documentation, in my mind documentation examples should be as simple as possible, but at any rate they should be correct.

Ref: https://tools.ietf.org/html/rfc7231#section-7.1.4

An origin server SHOULD send a Vary header field when its algorithm
for selecting a representation varies based on aspects of the request
message other than the method and request target, unless the variance
cannot be crossed or the origin server has been deliberately
configured to prevent cache transparency.

@nigoroll
Copy link
Author

@DamianRivas your example looks good to me, but I am not a node dev

mukulkhanna added a commit to mukulkhanna/node that referenced this issue Feb 26, 2019
Co-Authored-By: mukulkhanna <[email protected]>
@addaleax
Copy link
Member

This has been fixed by #26308.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants