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

fix: add warning about setting both useCdn and withCredentials to true #849

Merged
merged 4 commits into from
Jun 18, 2024

Conversation

amrfarid140
Copy link
Contributor

This is a proposed fix to address the minimum solution suggested for #833

@rexxars
Copy link
Member

rexxars commented Jun 10, 2024

Thanks for the PR! I see the intention behind this, and agree that a warning is warranted. However, the current warning message in this PR is:

Since you have set withCredentials to true, we will default useCdn to false.

I don't think this strongly enough conveys what actually happens – "default" sounds to me like "if you haven't set useCdn to anything", whereas what happens is actually more along the lines of

Because you set withCredentials to true, we will override your useCdn setting to be false since (cookie-based) credentials are never set on the CDN

Does that make sense?

I'll make a note that we should update the documentation/TSDoc on the withCredentials flag - it generally shouldn't be used, unless you know that you need studio cookie-based authorization headers to be sent along with the request. Perhaps we should also warn when passing both a token and withCredentials: true, as those indicate two different ways to pass authorization information to the API.

@amrfarid140
Copy link
Contributor Author

Yes the suggested message is more direct!. I've push an update with the new ticket and hopefully a fix for lint.

Signed-off-by: Cody Olsen <81981+stipsan@users.noreply.github.com>
Signed-off-by: Cody Olsen <81981+stipsan@users.noreply.github.com>
@stipsan stipsan enabled auto-merge (squash) June 18, 2024 10:44
@rexxars
Copy link
Member

rexxars commented Jun 18, 2024

Thank you @amrfarid140 - this is great ❤️

@stipsan stipsan merged commit ae01edb into sanity-io:main Jun 18, 2024
16 checks passed
@ecospark ecospark bot mentioned this pull request Jun 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants