cachapa / firedart

A dart-native implementation of the Firebase Auth and Firestore SDKs
https://pub.dev/packages/firedart
Apache License 2.0
174 stars 62 forks source link

Call notifyState on creating TokenProvider #133

Open nickmeinhold opened 5 months ago

nickmeinhold commented 5 months ago

Fixes #132

cachapa commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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