fix: prioritize setAuth action #1435
Conversation
* test: run integration tests in CI * run test:ingration twice * Install deno on integration job * use npm * run puppeteer with no sandbox * run browser integration test twice * trying to fix umd build command * no need to run browser integration twice
There was a problem hiding this comment.
Pull Request Overview
This PR aims to prioritize the setAuth action within the event loop by converting authentication event handling to an async model.
- Converted _listenForAuthEvents and _handleTokenChanged to async functions
- Introduced a setTimeout to defer the _handleTokenChanged call and updated realtime.setAuth to be awaited
| private async _listenForAuthEvents() { | ||
| return await this.auth.onAuthStateChange((event, session) => { | ||
| setTimeout( | ||
| async () => await this._handleTokenChanged(event, 'CLIENT', session?.access_token), |
There was a problem hiding this comment.
Consider wrapping the async callback in a try/catch block inside the setTimeout to handle potential errors from _handleTokenChanged, preventing unhandled promise rejections.
| async () => await this._handleTokenChanged(event, 'CLIENT', session?.access_token), | |
| async () => { | |
| try { | |
| await this._handleTokenChanged(event, 'CLIENT', session?.access_token) | |
| } catch (error) { | |
| console.error('Error in _handleTokenChanged:', error) | |
| } | |
| }, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
since the other suggested change is also try/catching inside the _handleTokenChanged, don't need to catch it here also.
| @@ -339,7 +341,8 @@ export default class SupabaseClient< | |||
| ) { | |||
| this.changedAccessToken = token | |||
| } else if (event === 'SIGNED_OUT') { | |||
| this.realtime.setAuth() | |||
| await this.realtime.setAuth() | |||
There was a problem hiding this comment.
Ensure that any potential failure from realtime.setAuth is handled, for example by adding error handling or logging in case the operation fails.
| await this.realtime.setAuth() | |
| try { | |
| await this.realtime.setAuth() | |
| } catch (error) { | |
| console.error('Failed to set auth for realtime client:', error) | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
For this case I think the most correct would be:
try {
await this.realtime.setAuth()
if (source == 'STORAGE') this.auth.signOut()
} catch (error) {
console.log('Failed to set auth for realtime client:', error)
} finally {
this.changedAccessToken = undefined
}wdyt @filipecabaco ?
There was a problem hiding this comment.
probably yes as it would also prevent running an old token against it... but will this break auth?
Pull Request Test Coverage Report for Build 15382986746Details
💛 - Coveralls |
What kind of change does this PR introduce?
Prioritizes setAuth action in the event loop