-
Notifications
You must be signed in to change notification settings - Fork 330
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(clerk-js): Only refresh when auth with popup is called #5518
Conversation
🦋 Changeset detectedLatest commit: a031b60 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// If `handleRedirectCallback` is called on a window without an opener property (such as when the OAuth flow popup | ||
// directs the opening page to navigate to the /sso-callback route), we need to reload the signIn and signUp resources | ||
// to ensure that we have the latest state. This operation can fail when we try reloading a resource that doesn't | ||
// exist (such as when reloading a signIn resource during a signUp attempt), but this can be safely ignored. |
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.
Could this entire thing be done via a postMessage()
from the popup? The issue is we don't know which resource we need to fetch so we're just doing both, right?
// If `handleRedirectCallback` is called on a window without an opener property (such as when the OAuth flow popup | ||
// directs the opening page to navigate to the /sso-callback route), we need to reload the signIn and signUp resources | ||
// to ensure that we have the latest state. This operation can fail when we try reloading a resource that doesn't | ||
// exist (such as when reloading a signIn resource during a signUp attempt), but this can be safely ignored. | ||
if (!window.opener) { | ||
if (!window.opener && shouldRefreshResources) { | ||
try { | ||
await signIn.reload(); |
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.
Can you remind me why doing .reload()
is useful, if it always returns a 405 ?
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.
It doesn't always return a 405. It only returns a 405 when the underlying resource doesn't exist. So for example if we created a SignIn resource during the popup, signIn.reload()
will pick that up, but signUp.reload()
will return a 405.
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.
oh i see, thanks for clarifying.
closing in favor of #5553 |
Description
This PR uses
sessionStorage
to set a flag indicating thatauthenticateWithPopup
was used to initiate an SSO connection, which likely means that thesignIn
andsignUp
resources need to be reloaded before handling the redirect.This reduces the instances of a
405 Method Not Allowed
response from FAPI which is returned when we attempt to refresh a resource that we haven't acted upon. For example, if we're attempting to sign in, callingsignUp.reload()
will cause an HTTP 405 error. By only performing this reload whenauthenticateWithPopup
has been called, we reduce the instance of 405 errors in our logs and within customer applications.Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change