fix(clerk-js,types): Use intent parameter to reload resource #5553
Conversation
🦋 Changeset detectedLatest commit: e06e243 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 ↗︎
|
| @@ -24,7 +24,9 @@ export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCal | |||
| React.useEffect(() => { | |||
| let timeoutId: ReturnType<typeof setTimeout>; | |||
| if (__internal_setActiveInProgress !== true) { | |||
| handleRedirectCallback({ ...props }, navigate).catch(e => { | |||
| const intent = new URLSearchParams(window.location.search).get('intent'); | |||
There was a problem hiding this comment.
Should we maybe use the params from the router instead ?
There was a problem hiding this comment.
In the hash and virtual routers, the queryString isn't derived from window.location.search, which is where the intent param is going to be. For example, this is the SSO callback route when using the hash router:
http://localhost:4000/sign-in?intent=sign-in#/sso-callback
In this example, window.location.search is ?intent=sign-in, but the value of queryString from useRotuer is "".
Alternatively, we could append the intent parameter to different locations based on the router used, but I felt that was a little too much additional complexity in exchange for basically no additional functionality.
| @@ -24,7 +24,9 @@ export const SSOCallbackCard = (props: HandleOAuthCallbackParams | HandleSamlCal | |||
| React.useEffect(() => { | |||
| let timeoutId: ReturnType<typeof setTimeout>; | |||
| if (__internal_setActiveInProgress !== true) { | |||
| handleRedirectCallback({ ...props }, navigate).catch(e => { | |||
| const intent = new URLSearchParams(window.location.search).get('intent'); | |||
| const reloadResource = intent === 'signIn' || intent === 'signUp' ? intent : undefined; | |||
There was a problem hiding this comment.
Can we do this ? Or is reloadResource:false not proper ?
| const reloadResource = intent === 'signIn' || intent === 'signUp' ? intent : undefined; | |
| const reloadResource = ['signIn', 'signUp'].includes(intent) |
There was a problem hiding this comment.
TypeScript won't narrow from string to 'signIn' | 'signUp' if we use Array.includes, and we still need to make sure we know which resource needs to be reloaded, so a boolean value won't work.
Description
This PR introduces a new URL parameter,
intent, used when performing SSO login via popup to indicate which resource needs to be refreshed before handling the redirect callback. Theintentparameter is appended to theredirectUrlpassed to FAPI such that the popup flow redirects to theredirectUrlwith theintentparameter set to the resource that needs to be reloaded. This is passed to a newreloadResourceoptional parameter on thehandleRedirectCallbackmethod to indicate which resource to reload.This reduces the instances of a
405 Method Not Allowedresponse 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 on thesignInresource, we reduce the instance of 405 errors in our logs and within customer applications.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change