-
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(app): annotate initializeApp return as a promise #8366
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
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.
Unfortunately we can't accept this change
1- it must return AppCheck, not FirebaseApp (note! currently this is an implementation change, but it's an important one and we need to do it ASAP
2- it should not return a Promise, that's a difference from firebase-js-sdk and there's no technical reason to do it
reference ref: https://firebase.google.com/docs/reference/js/app-check.md#initializeappcheck_5548dfc
Separately: currently it is not returning a Promise anyway as far as I can tell?
1- getApp() (underlying source of the FirebaseApp that it is returning, though it should return AppCheck, is not async: https://github.com/invertase/react-native-firebase/blob/main/packages/app/lib/modular/index.d.ts#L55
2- implementation delegates but is not async https://github.com/invertase/react-native-firebase/blob/main/packages/app/lib/modular/index.js#L71
3- fundamental implementation is not async for getApp
export function getApp(name = DEFAULT_APP_NAME) { |
So, I don't think it is a Promise currently, curious how you're seeing that
Regardless, cannot be Promise and cannot return FirebaseApp to correctly implement the firebase-js-sdk and meet our goal of being a drop-in replacement for firebase-js-sdk
You're linking the source for It is already annotated as such in the types for the namespaced API, the modular types are just inconsistent: https://github.com/invertase/react-native-firebase/blob/main/packages/app/lib/index.d.ts#L198 Maybe this is inconsistent with firebase-js-sdk, but if so it's not a new inconsistency in the implementation. |
Description
initializeApp
in the modular API is annotated is returning the app directly, but like in the namespaced API it actually returns a promiseRelated issues
Release Summary
Checklist
Android
iOS
Other
(macOS, web)e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
N/A