-
Notifications
You must be signed in to change notification settings - Fork 59
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 origin on delegated options #292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@saschanaz could you review as well? How much of this PR is reverting the original change? |
The spirit of the original change is still there, that when we have a static or '*' origin then we should not add the vary header. It's just that dynamic config through a callback can make the origin dynamic. The other dynamic config possibilities was handled in the original PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in terms of the behavior, as my intention of the patch was only for static options.
Sorry for the churn and thank you for the fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
} | ||
}) | ||
} | ||
|
||
/** | ||
* @param {import('./types').FastifyCorsOptions} opts | ||
*/ | ||
function normalizeCorsOptions (opts) { | ||
function normalizeCorsOptions (opts, dynamic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function normalizeCorsOptions (opts, dynamic = false) {
@@ -148,11 +148,12 @@ function normalizeCorsOptions (opts) { | |||
// strings are applied directly and any other value is ignored | |||
corsOptions.cacheControl = null | |||
} | |||
corsOptions.dynamic = dynamic || false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late to the party:
why didn't we set it during the declaration?
@Eomm |
Yeah, but it is not released 😈 |
Can it be released soon though? |
shipped |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@fastify/cors](https://github.com/fastify/fastify-cors) | [`9.0.0` -> `9.0.1`](https://renovatebot.com/diffs/npm/@fastify%2fcors/9.0.0/9.0.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@fastify%2fcors/9.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@fastify%2fcors/9.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@fastify%2fcors/9.0.0/9.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@fastify%2fcors/9.0.0/9.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify-cors (@​fastify/cors)</summary> ### [`v9.0.1`](https://github.com/fastify/fastify-cors/releases/tag/v9.0.1) [Compare Source](https://github.com/fastify/fastify-cors/compare/v9.0.0...v9.0.1) #### What's Changed - vary origin on delegated options by [@​laat](https://github.com/laat) in [https://github.com/fastify/fastify-cors/pull/292](https://github.com/fastify/fastify-cors/pull/292) #### New Contributors - [@​laat](https://github.com/laat) made their first contribution in [https://github.com/fastify/fastify-cors/pull/292](https://github.com/fastify/fastify-cors/pull/292) **Full Changelog**: fastify/fastify-cors@v9.0.0...v9.0.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/tomacheese/telcheck). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
When the options used are delegated to a function, then the origin could be dynamic and should add
vary: origin
header.The symbol usage feels a bit hacky, but the test changes should be ok.
fixes #291
Checklist
npm run test
and the Code of conduct