Skip to content

Fix: Realtime setup with GoTrue v2 #468

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

Merged
merged 4 commits into from
Jul 1, 2022
Merged

Fix: Realtime setup with GoTrue v2 #468

merged 4 commits into from
Jul 1, 2022

Conversation

alaister
Copy link
Member

What kind of change does this PR introduce?

Fix

What is the current behaviour?

The current setup may not set an access token until the token is refreshed. This is because of a race condition where if the initial SIGNED_IN event occurs while the channel is in an isJoining state, it won't send the access token.

What is the new behaviour?

This setup sets the join token if we have it, and always sets the access token after the channel has joined successfully.

Additional context

It's worth mentioning in the docs that there may (rarely) be a case where realtime connects with the anon token even when the user is logged in before it sends up the current users token.

@alaister alaister requested a review from w3b6x9 June 29, 2022 06:48
this.fetch = fetchWithAuth(supabaseKey, this._getAccessToken.bind(this), settings.fetch)

this.auth = this._initSupabaseAuthClient(settings.auth || {}, this.headers, this.fetch)
Copy link
Member Author

Choose a reason for hiding this comment

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

@soedirgo fyi this was a circular reference. Is there any reason why we can't use the regular settings.fetch here instead of our fetchWithAuth?

Copy link
Member

Choose a reason for hiding this comment

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

looks like the circular reference is this.auth -> this.fetch -> this._getAccessToken -> this.auth

not quite familiar with gotrue-js but I can't think of any reason not to use settings.fetch off the top of my head (but we def can't use it for functions and storage)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. It's only if we were doing something extra in fetchWithAuth that gotrue-js needed... but yeah it just does a normal fetch so thanks, Bobbie!

Copy link
Member

@w3b6x9 w3b6x9 left a comment

Choose a reason for hiding this comment

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

@alaister realtime changes lgtm!

@alaister alaister merged commit 53decab into next Jul 1, 2022
@alaister alaister deleted the fix/gotrue2-realtime branch July 1, 2022 07:00
@w3b6x9
Copy link
Member

w3b6x9 commented Jul 1, 2022

It's worth mentioning in the docs that there may (rarely) be a case where realtime connects with the anon token even when the user is logged in before it sends up the current users token.

@alaister good point! will add to backlog.

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.

3 participants