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 Header should not be controlled by the client #138

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skryukov
Copy link
Contributor

The Vary header is a way for servers to control caching mechanisms, and it is purely a server-side mechanism. The client does not control or affect the Vary header. Issue #136 introduced logic that does not conform to this: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-semantics-19#section-12.5.5

There are several approaches we can take with the Vary header:

  1. Apply the original Laravel plugin logic: force the Vary: X-Inertia header.
  2. Set the Vary header if it wasn't set before, which is the default logic used in Rails: https://github.com/rails/rails/blob/b8720e7cb8c9894d142b8576c21065c92cd1907a/actionpack/lib/action_controller/metal/rendering.rb#L220-L224
  3. Append X-Inertia if the Vary header was set previously, or set the Vary: X-Inertia header if it was not.

I believe the third option makes the most sense: it allows users to add other headers, like Accept-Language, to the Vary list, but it also prevents them from omitting X-Inertia.

@@ -16,7 +16,11 @@ def initialize(component, controller, request, response, render_method, props: n
end

def render
@response.set_header('Vary', [@request.headers['Vary'], 'X-Inertia'].compact.join(', '))
if @response.headers["Vary"].blank?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I prefer the original functional approach, but it generates unnecessary objects, which can affect production performance due to additional GC load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant