BREAKING: Remove SupabaseAuthRequiredState as well as overriding methods in SupabaseAuthState

[dshukertjr]

Comments are welcome!

This is just a proposal in an attempt to improve developer experience of this library.

Current behavior

Currently, there are SupabaseAuthState and SupabaseAuthRequiredState. A lot of their functionalities overlap, and having two different classes might be confusing.

Also, SupabaseAuthState have the following methods, but users don't necessary need all of them if they are using SupabaseAuthState just for auth state persistance.

  /// Callback when deeplink received and is processing. Optional
  void onReceivedAuthDeeplink(Uri uri) {
    Supabase.instance.log('onReceivedAuthDeeplink uri: $uri');
  }

  /// Callback when user is unauthenticated
  void onUnauthenticated();

  /// Callback when user is authenticated
  void onAuthenticated(Session session);

  /// Callback when authentication deeplink is recovery password type
  void onPasswordRecovery(Session session);

  /// Callback when recovering session from authentication deeplink throws error
  void onErrorAuthenticating(String message);

New behavior

All of the classes that were inheriting State has been removed, and deeplinking and auth state persistence logic has been consolidated into SupabaseAuth class.

Instead of relying on onAuthenticated() and onUnauthenticated(), users can utilize Supabase.instance.client.auth.onAuthStateChange() to navigate users to different routes depending on Auth state.

[dshukertjr]

Also thinking that we could handle startAuthObserver() in a smarter way as well.

[bdlukaa]

tests are failing

[dshukertjr]

@bdlukaa I haven't changed the tests yet, but I wanted to see what you and other people think about the general direction of this PR.

[dshukertjr]

@DanMossa @Vinzent03
It would be great if I could get some feedbacks on the general direction of this PR. Would you agree that this is a good change?

[Vinzent03]

I'm currently not using this library in my Flutter app. Instead I'm using supabase-dart. I don't know exactly why I didn't choose the flutter version, but I think the weird state classes were one reason. Using the listener directly is a good idea. To understand the new class a bit more:
Should I place the class multiple times in my widget tree or just one time at the top?

[bdlukaa]

Imo we should get rid of all these classes and let the dev do it

[DanMossa]

I think helper classes like this are only useful if they're basically required to be used.

Like, if 99% of people are going to use onUnauthenticated then that's awesome, but if there's other alternatives then I think it gets confusing.

My personal opinion is there should be 1 good way of doing something or else there's too much concern on which way the user should be doing it.

[dshukertjr]

@Vinzent03 That is funny, because I do the same thing lol I use only supabase-dart in my app.

Should I place the class multiple times in my widget tree or just one time at the top?

Yeah, the currently the idea is to have only one of them alive in a widget tree at once.

@bdlukaa

Imo we should get rid of all these classes and let the dev do it

I agree. Having these classes are counter intuitive for Flutter devs and causes confusion. One thing that I would want is automatic token refreshing though. It would be nice to provide a way where developers don't have to think about refreshing tokens themselves, and in order to do that, we need to listen to app's lifecycle. The way we do it currently is not the most elegant way of doing it, but do you think the developer should manage token refreshing by themselves too? Or should we explore better way of automatically handling token refresh without creating an extended class from State?

@DanMossa
100% agree with what you are saying here. To me, this class that we provide is only good for automatically refreshing the auth token as well as handling deep links, but as I wrote earlier, I think we might be able to come up with a better way of handling automating token refresh without creating a class that extends State.

@Vinzent03 @bdlukaa @DanMossa
I think these are the questions that we want to ask ourselves.

• Should we provide a way to handle deeplinks and automatic token refresh, or should each developers take care of them on their own?

-> In my opinion, not having to think about token refresh is a nice developer experience, and quite frankly something that developers expect especially if they come from Firebase world, but open to suggestions.

• If we are going to provide a way to automatically refresh token and handle deeplinks, how should we do it?

-> I think with this PR, all developers have to do is to have single SupabaseAuthState alive somewhere within their widget tree, and that is all they need to do, so it is considerably simpler than the current implementation, but ideal world would be if the user installs supabase-flutter package and the auth state is persisted with no additional configuration. I will do some research on how we could possibly do this, but would any of you have any suggestions on how that could be achieved? I think as long as there is a way of listening to app's lifecycle changes, we can automatically perform all the token refresh and stuff.

[Vinzent03]

@dshukertjr I think automatic token refreshing is really important. Callbacks for onAuthenticated or onErrorAuthenticating should be removed and not handled in the widget tree.
Listening to WidgetsBindingObserver does not directly need context or a widget, so it could be integrated in the flutter client without an extra widget. Disposing and initializing must be handled manually then.

[dshukertjr]

@Vinzent03 I did not know that WidgetsBindingObserver can be used without it being in the widget tree! That's great!

Rewriting the PR as soon as I get back in front of a keyboard!

[Vinzent03]

@dshukertjr I haven't tested it, but following this Stack Overflow answer it should work.

[Vinzent03]

@dshukertjr Looks great, but there is still SupabaseAuthState for deep linking, right? What about moving this out of the widget tree too?

[dshukertjr]

@Vinzent03 Thanks for the quick checkup! Yup, will move the deep link stuff out of the widget tree too!

[dshukertjr]

Actually testing out deeplink on Android and ios now...

[dshukertjr]

Added _startDeeplinkObserver here so that the user of this package don't explicitly have to call to start deep link observer. I think it shouldn't have much of an effect for those that do not use deeplinks, but what does everyone think about this?

[DanMossa]

Question to make sure I'm following this logic correctly:

1. Supabase gets initialized.
2. The instance then starts checking to see if the app is being opened from any deep links, _handleDeepLink is called with then calls recoverSessionFromUrl.
3. We recover the session from the url.

I think that's fine, but supabase doesn't need to try to recoverSessionFromUrl for every deeplink that opens the app.

I'd want _instance.isAuthCallbackDeeplink to be better at checking to see if it's actually a url that we want to recover a session from.

[dshukertjr]

I think most of the rework is done.

One thing that I would want to add is a way to know when the initial authentication check is complete.

In an app, it is pretty common to show different things to the users depending on whether the user is logged in or not. Currently with this rework, there is no way of knowing when the initial auth check has been complete. If the user is logged in, onAuthStateChange will run when the token refresh is complete, but if the user is not logged in, nothing will happen. There is no way to tell if the user is not signed in, or is just waiting on token refresh.

What would be the best way to give developers a way to know when the initial auth state? One idea that came to my mind was to have a completer name something like hasReceivedInitialAuthState or something, and users can await for the future of the completer, but I'm sure all of you have better ideas, so would love to hear them!

[dshukertjr]

I think this PR is pretty much complete on my end. Idealy, we will merge supabase/gotrue-dart#74 and supabase/postgrest-dart#69 and bump the major version of this package to release all of these breaking changes!

[bdlukaa]

LGTM overall!

[rlee1990]

@dshukertjr how would this work with say flutter riverpods? I was using firebase before and had a Stream but I see this uses GotrueSubscription.

[dshukertjr]

@rlee1990
You should be able to use it very similar to how you are currently using Firebase!
By using auth.onAuthStateChange(), you can listen to auth state changes just like listening to auth state change in Firebase. There is two values that you can listen to, the event and state, so there it is not using Stream, but the basic functionality is the same! Do you have any specific concerns?

[rlee1990]

@dshukertjr Thanks for the quick reply. I currently use river pods flutter with a streamProvider. I will just need to see how I am going to use this in a different way with autoRouter and riverPods. Also was the deep link parts removed?

[dshukertjr]

@rlee1990
If you are using StreamProvider, you could probably yield a new stream every time the auth state changes on auth.onAuthStateChange() listener!

Deep link related logic are not removed, but have been moved inside supabase_auth.dart file!