-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add initial event to onAuthStateChange #147
Conversation
looks like the failing tests are from gotrue - not your changes. Let me investigate |
The format of the |
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.
Seems like consumers can't know if the user is really logged out or it's just a delayed auth initialisation if they attach onAuthStateChanged
after the initial event has been sent.
Should every new listener be notified of the current auth state?
Not sure if people around here like comparisons, but I immediately recognised this API coming from Firebase, and that's how it works over there 😅
Adding a bit more food for thought on the above: |
Maybe inside |
Has this change been given more consideration? Would love to see it implemented |
@@ -86,17 +86,27 @@ export default class GoTrueClient { | |||
headers: settings.headers, | |||
cookieOptions: settings.cookieOptions, | |||
}) | |||
|
|||
// an array of promises which load the current user | |||
let initPromises: Array<Promise<any>> = [] |
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.
Could all this be simplified to
Promise.all([
this._recoverSession(),
this._recoverAndRefresh(),
this.getSessionFromUrl({ storeSession: true })
])
.then(([, , { error }]) => {
if (error) {
console.error("Error getting session from URL.", error)
}
})
.catch((error) => {
console.error("Error recovering session.", error)
// Any other error handling...
})
.finally(() =>
this._notifyAllSubscribers("INITIALIZED") // Other events appear to be past-tense.
)
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.
#147 (comment) @gustavohenke Alternatively the client could present // ...
class GoTrueClient {
private _isInitialized: boolean = false
public constructor() {
// ....
Promise.all([
this._recoverSession(),
this._recoverAndRefresh(),
this.getSessionFromUrl({ storeSession: true })
])
.then(([, , { error }]) => {
if (error) {
console.error("Error getting session from URL.", error)
}
})
.catch((error) => {
console.error("Error recovering session.", error)
// Any other error handling...
})
.finally(() => {
this._notifyAllSubscribers("INITIALIZED") // Other events appear to be past-tense.
this._isInitialized = true
})
}
public async isInitialized(): Promise<boolean> {
if (this._isInitialized) {
return true
}
return new Promise((resolve) => {
this.onAuthStateChange((event) => {
if (event === "INITIALIZED") {
return resolve(true)
}
})
})
}
} |
Thank you for this contribution, however the library has advanced quite a bit. |
What kind of change does this PR introduce?
Fixes #326
What is the current behavior?
There is no event fired unless a sign in occurs, but that is sometimes delayed. So there is no way to know if the user is signed in or not right when the page loads
What is the new behavior?
There is an
INITIAL
event fired after the library is finished loading and checking for a logged in user.