-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
feat: add skipBrowserRedirect
option to signInWithOAuth
#575
Conversation
@@ -343,6 +343,7 @@ export default class GoTrueClient { | |||
redirectTo: credentials.options?.redirectTo, | |||
scopes: credentials.options?.scopes, | |||
queryParams: credentials.options?.queryParams, | |||
skipBrowserRedirect: credentials.options?.skipBrowserRedirect, |
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.
I think this might be slightly confusing for some because some might interpret the "redirect" here as the redirect after the oauth flow has completed when we're actually referring to the redirect to the oauth provider's page.
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.
Yeah let's merge this next year (hehe) to see if I can come up with a better name.
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.
I assume the objective here is to eventually sign the user in via oauth, but not yet? And that the dev is now responsible for triggering the oauth provider url, which is returned.
Other possibilities:
delayProviderSignIn
enableMiddleware
- if that's why this option is being introduced
autoAuthorization
- would need a default of true
autoProviderSignIn
- would need a default of true
autoSignIn
- would need a default of true
. might be more scalable if you ever wanted to introduce similar behavior for other sign-in methods.
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.
I assume the objective here is to eventually sign the user in via oauth, but not yet?
Yup, the purpose is to return the oauth provider's URL so the developer can decide how they want to redirect the user to the site. I'd also prefer for the naming to be more explicit such that it encompasses the meaning of the oauth provider URL being returned.
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.
Thanks for the clarification.
I'd also prefer for the naming to be more explicit such that it encompasses the meaning of the oauth provider URL being returned.
I'm having trouble coming up with verbiage to express this - especially since the URL is returned no matter what.
44b8dfa
to
9112e8c
Compare
I don't think we came up with any better name, so I'll merge it. |
🎉 This PR is included in version 2.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Allows for custom handling of the redirect URL when in browser environments.
Fixes: #417