Skip to content
This repository has been archived by the owner on May 13, 2023. It is now read-only.

fix: Use BehaviorSubject for _onAuthStateChangedController #116

Merged
merged 1 commit into from
Jan 16, 2023

Conversation

dshukertjr
Copy link
Member

What kind of change does this PR introduce?

Currently, there are no ways to listen to the very first recovery event through onAuthStateChanged stream, because Supabase.initialize() completes after after the recovery event is handled.

This PR changes the _onAuthStateChangedController to BehaviorSubject so that the last event is preserved and can be listened to even after the Supabase.initialize() is completed.

Related supabase/supabase-flutter#331

Other context

The commit message got mixed up with my previous task, so please ignore it 😂

@Vinzent03
Copy link
Contributor

Isn't initialSession designed for that?

@dshukertjr
Copy link
Member Author

@Vinzent03
I wasn't happy with the implementation of SupabaseAuth.initialSession for a few reasons.

  • It feels odd to use SupabaseAuth.initialSession to get the initial session and auth.currentState for the rest
  • It is the only time a developer uses SupabaseAuth class through out the usage of supabase-flutter.

To me, the main purpose of SupabaseAuth.initialSession was to provide a way for developers to catch the error that happens upon session recovery or obtaining session from deep links. With this PR, users can just set onError listener to onAuthStateChanged to get the initial error as well, and I think SupabaseAuth.initialSession can be deprecated.

@bdlukaa
Copy link
Contributor

bdlukaa commented Jan 13, 2023

I believe you can do that by calling onAuthStateChange.first

@dshukertjr
Copy link
Member Author

@bdlukaa
I believe .first awaits for the first stream emitted after calling it, and does not return the previous value.

Currently, this is what is happening.

  1. Call await Supabase.initialize(), which does not complete until session recovery or handling initial deep link is done.
  2. User can set a listener to supabase.auth.onAuthStateChanged and start listening to auth state changes.

At event 1, any auth event emitted during initialize() can only be retrieved using SupabaseAuth.initialSession, and cannot be retrieved using onStateChanged. I think being able to listen to auth state just by subscribing to onAuthStateChanged provides a more consistent API for the developer, but what do you think?

@bdlukaa
Copy link
Contributor

bdlukaa commented Jan 15, 2023

I believe .first awaits for the first stream emitted after calling it, and does not return the previous value.

.last would do it.


To me, supabase.auth.onAuthStateChanged should only be called when any auth state is updated, otherwise it can cause inconsistency between code. Imagine listening to a stream and receiving events that have already happened.

In a Flutter context, this stream would only be used to update the screen. If a StreamBuilder is used, it'll use the last value from the stream.

@aguilaair
Copy link

imagen

last does not seem to work

@dshukertjr
Copy link
Member Author

@bdlukaa
last only returns a value once returns a value once the stream is closed, so in our case, it cannot be used.
dart-lang/sdk#35211 (comment)

There are no way of obtaining the value of the stream that was emitted previously in the default StreamController class I believe.

To me, supabase.auth.onAuthStateChanged should only be called when any auth state is updated, otherwise it can cause inconsistency between code. Imagine listening to a stream and receiving events that have already happened.

I do agree that this could be a problem, but

In a Flutter context, this stream would only be used to update the screen. If a StreamBuilder is used, it'll use the last value from the stream.

if this is what you think is the ideal behavior, then this PR would enable it.

Copy link
Contributor

@bdlukaa bdlukaa left a comment

Choose a reason for hiding this comment

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

sounds ok to me

@Vinzent03
Copy link
Contributor

@dshukertjr Have you tested that it "caches" the latest error as well for new listeners? I don't know if it may only do that for data events.
To be honest, I'm still not sure if this may break some implementations. I don't think you only use the stream for a stream builder. You may attach the listener for a new route or something and receive a new sign-out event, although that's already old.

@dshukertjr
Copy link
Member Author

@Vinzent03
Yup, persist the value or error, whichever is the latest. You can see a sample here.

I do understand your concern. But at the same time, I feel like the behavior in this PR is the behavior that people expect when listening to auth states as a stream as I have received few feedbacks about it. At least I think Firebase does it this way if I'm not wrong (It's been a while since I have used Firebase auth, so I could be wrong 😂).

@Vinzent03
Copy link
Contributor

Okay that's great.

I just tested with Firebase and you are right they emit the latest event on listening. So LGTM!

@dshukertjr dshukertjr merged commit 86dc72f into main Jan 16, 2023
@dshukertjr dshukertjr deleted the fix/initialEvent branch January 16, 2023 12:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants