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

feat: local provider prevent signIn flow #615

Merged
merged 10 commits into from Jan 8, 2024

Conversation

vettndr
Copy link
Contributor

@vettndr vettndr commented Dec 18, 2023

Closes #614.

I'm glad to discuss further improvements.
@zoey-kaiser

*
* @default false
*/
stopLogin?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something we only want to use for the registration flow, but adding it here, would also allow you to pass it for the signin flow. I think we need to find a way to:

  • Not modify the signin options (as for registration these are then passed onto the signIn function
  • We don't want to change the order of props that can be passed (to not make breaking changes)

I think we need to either:

  • Expand the signInOptions type by creating a registerOptions type with the added value
  • pass the value as a third optional parameter

Also lets rename it to preventLoginFlow

method,
body: credentials
})

if(signInOptions?.stopLogin) {
return response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not return different values based on if you want the signIn flow to happen or not, as I feel like this would lead to confusion.

@vettndr
Copy link
Contributor Author

vettndr commented Dec 19, 2023

@zoey-kaiser
thank you for the feedbacks.
I've just pushed some improvements as you suggested me. 🚀

Copy link
Member

@zoey-kaiser zoey-kaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for addressing my first review, I hope everything made sense 😊

const nuxt = useNuxtApp()

const { path, method } = useTypedBackendConfig(useRuntimeConfig(), 'local').endpoints.signUp
await _fetch(nuxt, path, {
await _fetch<Record<string, any>>(nuxt, path, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add types to _fetch here? Generally speaking I am not a fan of adding any type with any as it is pretty ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am not fan too.
It's a refuse coming from the previous changes, I've added just like in the n.19 line.
I'm going to delete it 🚀

@@ -105,7 +105,7 @@ const getSession: GetSessionFunc<SessionData | null | void> = async (getSessionO
return data.value
}

const signUp = async (credentials: Credentials, signInOptions?: SecondarySignInOptions) => {
const signUp = async (credentials: Credentials, signInOptions?: SecondarySignInOptions, registerOptions?: RegisterOptions) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to bug you one last time, but lets rename all of the options from registerOptions to signUpOptions to be more inline with the function name and then we are good to go 🥳

@root5427
Copy link

root5427 commented Jan 8, 2024

Hey folks! Huge thanks to @vettndr and @zoey-kaiser for their fantastic contributions! 🌟 Any updates on when these changes will be merged? 🚀

@zoey-kaiser zoey-kaiser merged commit cac3ce2 into sidebase:main Jan 8, 2024
4 checks passed
@zoey-kaiser
Copy link
Member

Hi @root5427 👋

We generally do our updates on Mondays! You can now use this in version https://github.com/sidebase/nuxt-auth/releases/tag/0.6.4!

@vettndr
Copy link
Contributor Author

vettndr commented Jan 9, 2024

@zoey-kaiser
Yeah! I'm so happy for this merge. 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Local provider: prevent signIn flow returned by signUp method
3 participants