-
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(messaging, ios): ensure getInitialNotification returns null vs undefined per types #5926
Conversation
This pull request is being automatically deployed with Vercel (learn more). react-native-firebase – ./🔍 Inspect: https://vercel.com/invertase/react-native-firebase/CUXcusK2HQZv89Y9m7kvKRAfb8Ad react-native-firebase-next – ./website_modular🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/FLSnQRJwAqbi4N2xwr31zbDDaQXT [Deployment for fdae945 canceled] |
a4b25f4
to
fdae945
Compare
undefined
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 - thank you! Shame there's no single doc, I should really PR something to the reactnative.dev site...would be handy
Codecov Report
@@ Coverage Diff @@
## main #5926 +/- ##
=============================================
+ Coverage 24.61% 52.63% +28.02%
- Complexity 0 625 +625
=============================================
Files 97 208 +111
Lines 4230 10157 +5927
Branches 1026 1612 +586
=============================================
+ Hits 1041 5345 +4304
- Misses 2587 4556 +1969
+ Partials 602 256 -346 |
was having issues getting this to run through CI for various unrelated reasons. I just pulled it and ran it locally, everything checks out, thanks again! |
Completely overlooked the merge notification! Thanks for reviewing and merging so quickly! |
Description
On iOS,
getInitialNotification
inRNFBMessaging+UNUserNotificationCenter.m
can benil
sogetInitialNotification
can beundefined
. However TS definition says it isPromise<RemoteMessage | null>
.I first was thinking of correcting this type definition, however as it will be a breaking change, fixed the proxy layer instead.
Related issues
None.
Release Summary
None.
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Automated tests should cover already.
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter