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.
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.
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.
"custom" was used because firebase calls it a custom token everywhere in the docs and their sign in function is called signInWithCustomToken()
requires bump of hanzo/auth