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

feat: Add callback for configuring Vary header #318

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gl-jkeys
Copy link

Fixes #317

This PR adds a callback (shouldSetVaryHeader) to the options which is used to determine when and how to configure the Vary header.

To preserve backward compatibility with all existing cors usages, this callback returns true by default.

If this PR is accepted, I will add documentation to the README on how to use this PR's functionality.

@UlisesGascon
Copy link
Member

I enabled the CI to run in this PR 👍

test/test.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@IamLizu
Copy link
Member

IamLizu commented Sep 15, 2024

Hey @gl-jkeys 👋, first of all, thank you for being an awesome contributor.

If this PR is accepted, I will add documentation to the README on how to use this PR's functionality.

It makes more sense if that README change is also included in this PR. Could you please take care of that?

(Note to self: I was unable to run the tests due to my own PC issue, I will check the tests again sometime later soon).

@gl-jkeys
Copy link
Author

Hey @IamLizu, I made the changes you requested; I also modified the PR to unconditionally set the Vary: Origin header for OPTIONS requests. (It seems unlikely, in hindsight since this PR was opened, that OPTIONS requests would be cached without respect to Origin.) I also simplified the tests to match the updated implementation.

@IamLizu
Copy link
Member

IamLizu commented Oct 2, 2024

Hey @gl-jkeys sorry I missed this one.

I will run the tests in my pc and get back to you.

Copy link
Member

@IamLizu IamLizu left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I am in favor of having the option to not set Vary header where it could help avoiding unncessary cache fragmentation.

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.

Add ability to omit Vary: Origin header
5 participants