-
Notifications
You must be signed in to change notification settings - Fork 453
feat(clerk-js): Include current search params in handleRedirectCallback
#7600
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
Changes from all commits
cea6727
276a2cb
a133b7d
b422477
8e7497a
432c362
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/clerk-js': patch | ||
| --- | ||
|
|
||
| Automatically include current window search params (redirect_url) for AuthenticateWithRedirectCallback to make it easier to use for sso-callback |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2064,6 +2064,7 @@ export class Clerk implements ClerkInterface { | |
| signUp: SignUpResource; | ||
| navigate: (to: string) => Promise<unknown>; | ||
| }, | ||
| searchParams?: URLSearchParams, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add a quick test verifying that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I agree with not changing the GoogleOneTap behavior, I'm a little hesitant to add a test because I don't know what the behavior should be... I did do some manual tests and the behavior seemed unchanged regardless of the search params. Probably in part because with GoogleOneTap, my sample app never redirects and just stays on the custom sign in page. I suppose someone implementing both custom sign-in and GoogleOneTap and supporting OAuth integrations would figure out they need to handle that... If you think it's merited I can easily add the test. But, that's my thinking. Let me know your perspective. :-) |
||
| ): Promise<unknown> => { | ||
| if (!this.loaded || !this.environment || !this.client) { | ||
| return; | ||
|
|
@@ -2126,7 +2127,7 @@ export class Clerk implements ClerkInterface { | |
| buildURL({ base: displayConfig.signInUrl, hashPath: '/reset-password' }, { stringify: true }), | ||
| ); | ||
|
|
||
| const redirectUrls = new RedirectUrls(this.#options, params); | ||
| const redirectUrls = new RedirectUrls(this.#options, params, searchParams); | ||
|
|
||
| const navigateToContinueSignUp = makeNavigate( | ||
| params.continueSignUpUrl || | ||
|
|
@@ -2334,12 +2335,18 @@ export class Clerk implements ClerkInterface { | |
|
|
||
| const navigate = (to: string) => | ||
| customNavigate && typeof customNavigate === 'function' ? customNavigate(to) : this.navigate(to); | ||
|
|
||
| return this._handleRedirectCallback(params, { | ||
| signUp, | ||
| signIn, | ||
| navigate, | ||
| }); | ||
| // Pass the current window location search params to preserve and prioritize | ||
| // a redirect_url from the OAuth flow. Needed by AuthenticateWithRedirectCallback. | ||
| const searchParams = new URLSearchParams(window.location.search); | ||
| return this._handleRedirectCallback( | ||
| params, | ||
| { | ||
| signUp, | ||
| signIn, | ||
| navigate, | ||
| }, | ||
| searchParams, | ||
| ); | ||
| }; | ||
|
|
||
| // TODO: Deprecate this one, and mark it as internal. Is there actual benefit for external developers to use this ? Should they ever reach for it ? | ||
|
|
||
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.
Would it be worth adding a test where both props and search params are present? Just to lock the precedence behavior (search params > props) and prevent future regressions
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.
Yes, good idea. Done.