Skip to content

fix: respect server.headers in static middlewares#8481

Merged
patak-cat merged 1 commit intomainfrom
fix/missing-headers
Jun 7, 2022
Merged

fix: respect server.headers in static middlewares#8481
patak-cat merged 1 commit intomainfrom
fix/missing-headers

Conversation

@patak-cat
Copy link
Copy Markdown
Member

Description

@jrvidal is working in a downstream integration and noticed that server.headers aren't being respected for several middlewares.

This change seems correct to me given Vite's docs: https://vitejs.dev/config/#server-headers
We may need to explore giving a functional form to define headers dynamically, but if a static list is exposed, they should be applied to all responses.

We should also review how headers are being respected in vite preview, but this is out of the scope of the PR


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-cat patak-cat added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Jun 6, 2022
Copy link
Copy Markdown
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

seems reasonable to me at least as a 2.9 fix

For v3, I'm honestly not quite sure the server.headers option is that useful because it's not typical you'd want to apply the same header to every response. You'll need to apply a header just to certain pages at least some of the time, so will need another way to apply headers resulting in two ways of doing things. I wonder if we should remove the option and tell users to apply the headers in their own middleware instead

@patak-cat
Copy link
Copy Markdown
Member Author

For v3, I'm honestly not quite sure the server.headers option is that useful because it's not typical you'd want to apply the same header to every response. You'll need to apply a header just to certain pages at least some of the time, so will need another way to apply headers resulting in two ways of doing things. I wonder if we should remove the option and tell users to apply the headers in their own middleware instead

I wonder why wasn't this the proposed solution. Does this work already? Maybe the issue is that Vite is setting its own headers so it will overwrite the one you add before? (and it isn't checking for the presence of prev headers)

@benmccann
Copy link
Copy Markdown
Collaborator

Does this work already?

I believe so. In SvelteKit, we expose a way for users to modify headers on a per-request basis, so that they can change them according to the URL or other properties of their choosing

@sapphi-red
Copy link
Copy Markdown
Member

It seems #5580 introduced this option. The intended use-case was modifying Cache-Control header (eg. adding stale-while-revalidate, no-store). Also it was suggested to use this for COOP and COEP headers.

@patak-cat
Copy link
Copy Markdown
Member Author

It seems #5580 introduced this option. The intended use-case was modifying Cache-Control header (eg. adding stale-while-revalidate, no-store). Also it was suggested to use this for COOP and COEP headers.

Do you think this is safe to merge then? The use case is COEP. The stale-while-revalidate seems fine to me, it is a header you may want to use depending on the request but I think it makes sense to be uniform here until we have a dynamic option

@sapphi-red
Copy link
Copy Markdown
Member

I think it is safe.

For v3, I am for this one.

I wonder if we should remove the option and tell users to apply the headers in their own middleware instead

@patak-cat
Copy link
Copy Markdown
Member Author

Ok, let's merge this one for now. PR welcome if you think docs or a deprecation is needed for v3. My stance about server.headers is that it is nice to have it as a convenience method, especially for things like COEP. I would prefer to keep it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants