-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix: making auth and firestore observable compatible #4078
Conversation
@@ -1634,6 +1634,8 @@ export namespace FirebaseAuthTypes { | |||
} | |||
} | |||
|
|||
type CallbackOrObserver<T extends (...args: any[]) => any> = T | { next: T }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No very sure on where to put this type
return typeof listenerOrObserver === 'object' | ||
? listenerOrObserver.next.bind(listenerOrObserver) | ||
: listenerOrObserver; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also not sure where to put this utils function.
Hi there! E2E test failure, possibly related? I haven't reviewed the PR yet - but was working on CI yesterday and wanted to see if the CI failure was false negative or real, so it's all I've examined so far:
|
Yeah, fixed now. |
also fix onSnapshot test
Nice, thanks for sending this over |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too - the test support exercising it is great, very much appreciated
@zhigang1992 - strange - I'm not sure if this is related to your change or not, but the current CI failure might be?
|
Working through a merge of all PRs that are ready, will take a while (probably in to tomorrow) to finish that, then I'll do the release. Thanks again @zhigang1992 ! |
* fix: observer compatibility in firestore * fix: add observer compatibility to auth APIs * fix: boolean * fix: tests * test: add auth test on onAuthStateChange also fix onSnapshot test * fix: lint Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> Co-authored-by: Mike Hardy <github@mikehardy.net>
* fix: observer compatibility in firestore * fix: add observer compatibility to auth APIs * fix: boolean * fix: tests * test: add auth test on onAuthStateChange also fix onSnapshot test * fix: lint Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> Co-authored-by: Mike Hardy <github@mikehardy.net>
Description
Official firebase-js-sdk support observer as a form of callback, we have partial support on Firestore's onSnapshot.
But it wasn't 100% inline with js-sdk, for example. previously, we were just passing observer.next directly. which will cause problem if that function has reference to observer via this. Adding bind(this) should do the trick
Related issues
#2066
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter