Skip to content
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

Call notifyState on creating TokenProvider #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nickmeinhold
Copy link

Fixes #132

@cachapa
Copy link
Owner

cachapa commented Jun 18, 2024

This causes a stream test to fail, I believe due to the extra event that is now being sent in the stream:

00:06 +13 -2: test/firebase_auth_test.dart: Emit signedIn events [E]           
  Expected: should do the following in order:
            • emit an event that satisfies function
            • emit an event that satisfies function
    Actual: <Instance of '_ControllerStream<bool>'>
     Which: emitted • false
              which didn't emit an event that satisfies function
  
  package:test_api  OutstandingWork.complete
  
  Expected: should do the following in order:
            • emit an event that satisfies function
            • emit an event that satisfies function
    Actual: <Instance of '_ControllerStream<bool>'>
     Which: emitted • false
              which didn't emit an event that satisfies function

Can you comment on this I'm concerned that this change would break existing implementations.

PS: please ignore the failing CI tests, they don't run properly on PRs because they need credentials that PR builds do not have access to.

@nickmeinhold
Copy link
Author

Yeah it would be a breaking change I guess. How about a new method on Token Provider, say

Stream<bool> get authStateChanges {
  _notifyState();
  return _signInStateStreamController.stream;
}

@nickmeinhold
Copy link
Author

I checked and while Flutter & Android fires an event on first listen (I couldn't find an iOS method), the JS SDK only triggers the observer when users are signed in & signed out.

So maybe there's no need to have a method that emits an event right after the listener has been registered. Unless the goal is to match the Flutter SDK?

Firebase Authentication on Flutter > authStateChanges()

Android > FirebaseAuth.AuthStateListener

Auth | JavaScript SDK > onAuthStateChanged

@cachapa
Copy link
Owner

cachapa commented Jun 20, 2024

Unless the goal is to match the Flutter SDK?

While the design for Firedart is inspired on the Firebase SDK, it already deviates significantly, and I don't feel a strong need to stick to it.

I checked and while Flutter & Android fires an event on first listen (I couldn't find an iOS method), the JS SDK only triggers the observer when users are signed in & signed out.

This is an interesting topic which I also went back and forth on, coming from other languages and frameworks.

Dart streams typically do not fire on listen. This makes some sense since broadcast streams would be either be re-firing duplicate events on each new listener, or would have different behaviour for the first listener and subsequent ones.

My solution for cases where I need the state on listen is to wrap the stream with a caching stream, e.g. RxDart's BehaviorSubject, and seed it with the starting value:

BehaviorSubject.seeded(auth.isSignedIn)
      ..addStream(auth.signInState)
      ..listen(print);

@nickmeinhold
Copy link
Author

Great thanks for the suggestion, shall we close this PR and issue then?

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.

Notify auth state on listen
2 participants