-
Notifications
You must be signed in to change notification settings - Fork 64
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
base: master
Are you sure you want to change the base?
Call notifyState on creating TokenProvider #133
Conversation
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. |
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;
} |
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() |
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.
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); |
Great thanks for the suggestion, shall we close this PR and issue then? |
Fixes #132