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

fix: making auth and firestore observable compatible #4078

Merged
merged 10 commits into from
Aug 15, 2020

Conversation

zhigang1992
Copy link
Contributor

@zhigang1992 zhigang1992 commented Aug 13, 2020

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@@ -1634,6 +1634,8 @@ export namespace FirebaseAuthTypes {
}
}

type CallbackOrObserver<T extends (...args: any[]) => any> = T | { next: T };
Copy link
Contributor Author

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;
}
Copy link
Contributor Author

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.

@mikehardy
Copy link
Collaborator

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:

  1) firestore().doc().onSnapshot()
       throws if SnapshotListenerOptions is invalid:
     Error: Did not throw an Error.
      at Context.<anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/e2e/DocumentReference/onSnapshot.e2e.js:302:29)
      at processImmediate (internal/timers.js:456:21)

@zhigang1992
Copy link
Contributor Author

zhigang1992 commented Aug 13, 2020

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:

  1) firestore().doc().onSnapshot()
       throws if SnapshotListenerOptions is invalid:
     Error: Did not throw an Error.
      at Context.<anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/firestore/e2e/DocumentReference/onSnapshot.e2e.js:302:29)
      at processImmediate (internal/timers.js:456:21)

Yeah, fixed now.

@Salakar
Copy link
Member

Salakar commented Aug 14, 2020

Nice, thanks for sending this over

Copy link
Collaborator

@mikehardy mikehardy left a 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

@mikehardy
Copy link
Collaborator

@zhigang1992 - strange - I'm not sure if this is related to your change or not, but the current CI failure might be?


  1) firestore().collection().onSnapshot()
       unsubscribes from further updates:
     AssertionError: expected Function { name: '' } to be called thrice but was called 4 times
    spy([object Object], null) at handleSuccess (http://localhost:8081/index.bundle?platform=android&dev=true&minify=false&sourceMapURL=true:202068:11)
    spy([object Object], null) at handleSuccess (http://localhost:8081/index.bundle?platform=android&dev=true&minify=false&sourceMapURL=true:202068:11)
    spy([object Object], null) at handleSuccess (http://localhost:8081/index.bundle?platform=android&dev=true&minify=false&sourceMapURL=true:202068:11)
    spy([object Object], null) at handleSuccess (http://localhost:8081/index.bundle?platform=android&dev=true&minify=false&sourceMapURL=true:202068:11)
  

@mikehardy
Copy link
Collaborator

Same thing happened on #1039 - I think it's a flaky test so I'm re-running the job and linking it to #4058 for attention

@mikehardy mikehardy merged commit d8410df into invertase:master Aug 15, 2020
@mikehardy
Copy link
Collaborator

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 !

androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
* 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>
androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Sep 15, 2021
* 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>
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.

3 participants