-
Notifications
You must be signed in to change notification settings - Fork 1
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
Auth: added support for common auth #94
Conversation
@@ -41,7 +41,7 @@ const LoginPanel: React.FC<PropsWithChildren & { | |||
className?: string, | |||
inputClx?: string, | |||
noHeading?: boolean | |||
onLoginChanged?: (loggedIn: boolean) => void | |||
onLoginChanged?: (token: string) => void |
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.
So there's a significant issue here re api design. The previous way indicated whether someone was logging in our out. With your method, the token sort of indicates that. But what if the API fails? It might be better in the following ways:
-
Let the client code and not the widget (sense a theme here??), handle what you do below. Originally this was just as intended as a way of creating alternative control flows. Let's keep it that way.
-
if we do keep the api call here, we shoudl prob return the "in/out" boolean as well as the status of the api. So the method could take both.
I would like to review this with you and come up w something potentially cleaner, post MVP. Please create an issue for this w the launch tag
@@ -69,6 +69,25 @@ async function getSession() { | |||
} | |||
} | |||
|
|||
export const generateCustomToken = async (): Promise<{success: boolean, token: string | null}> => { |
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.
Please avoid the term "custom" as much as possible. It expresses very little. If you find yourself tempted to use it, try to thing of the added functionality above the "non-custom" version that you're adding, and then name it accordingly. This is minor but I'd like to see it changed. Perhaps along a another ticket. Up to you how to track.
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.
"custom" was used because firebase calls it a custom token everywhere in the docs and their sign in function is called signInWithCustomToken()
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.
This is correct term.
requires bump of hanzo/auth