fix: sweep of firebase.json type config elements and related infrastructure #5597
Conversation
These had no correlation to settings available from the SDKs for toggling persistence on or off in Info.plist or AndroidManifest.xml, so should not have been in the firebase.json The javascript APIs exist and remain functional / untouched
…tion_enabled firebase.json setting This was a pending todo forever, and meant GDPR flow was not possible with in-app-messaging until now
android.it was working by coincidence, ios.it was not actually working, resulting in many unexpectedly skipped tests
Helps with GDPR flow for App Check users on iOS
|
This pull request is being automatically deployed with Vercel (learn more). react-native-firebase – ./🔍 Inspect: https://vercel.com/invertase/react-native-firebase/3amuXtFVdXcjdLkfxEcQfEa5DaRp react-native-firebase-next – ./website_modular🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/9MBTRCFprM8wBoVdpsC5SqhmvWBG |
Codecov Report
@@ Coverage Diff @@
## master #5597 +/- ##
============================================
- Coverage 71.11% 67.70% -3.41%
- Complexity 0 982 +982
============================================
Files 107 202 +95
Lines 4420 9929 +5509
Branches 942 1528 +586
============================================
+ Hits 3143 6721 +3578
- Misses 1179 2793 +1614
- Partials 98 415 +317 |
mikehardy
added
Workflow: Pending Merge
| @@ -247,6 +247,7 @@ - (NSDictionary *)constantsToExport { | |||
| if (@available(iOS 10.0, *)) { | |||
| if ([UIApplication sharedApplication].isRegisteredForRemoteNotifications == YES) { | |||
| resolve(@([RCTConvert BOOL:@(YES)])); | |||
| return; | |||
There was a problem hiding this comment.
@mikehardy This return is logically inconsistent with the comment 5 lines later
// Apple docs recommend that registerForRemoteNotifications is always called on app start
// regardless of current status
I don't know if the comment is accurate and I wasn't able to find any documentation from apple stating this when I just looked, but it is unclear to me now which cases we should call registerForRemoteNotifications.
There was a problem hiding this comment.
This was not intended to be any sort of a change that users should have to alter their code for. Whatever you have been doing, you should keep doing.
There have been other cases in this code base (and others I've been on) where resolving and then not returning has led to unintended side-effects as a resolve without return means code keeps executing.
In this case I judged the hygiene risk of that scenario much higher than attempting to register again, since by the conditional we are already registered.
That said, if you determine any case at all where your app does not seem to be registered, please let me know. That would be entirely unexpected (given the conditional being true in this case....)
There was a problem hiding this comment.
reading a bit further, it looks like the full/complete implementation of this Apple API would be implementing didRegisterForRemoteNotificationsWithDeviceToken and didFailToRegisterForRemoteNotificationsWithError and the resolving the promise with the result of them being called.
https://developer.apple.com/documentation/appkit/nsapplicationdelegate/1428766-application
I've found nothing yet to indicate if the app should always call registerForRemoteNotifications or if it can count on isRegisteredForRemoteNotifications and only call it registerForRemoteNotifications when it is false.
There was a problem hiding this comment.
Just noticed I'm using my personal account, this is all from brian@slashtalk.com, my mistake on not checking the browser tab.
There was a problem hiding this comment.
reading a bit further, it looks like the full/complete implementation of this Apple API would be implementing didRegisterForRemoteNotificationsWithDeviceToken and didFailToRegisterForRemoteNotificationsWithError and the resolving the promise with the result of them being called.
https://developer.apple.com/documentation/appkit/nsapplicationdelegate/1428766-application
That's a macOS API (NSApplication root).
I think you'll like that when we register we store the promise resolver and rejecter on our AppDelegate object, and we've swizzled ourselves into the response chain for when certain items are invoked, and we do what you mention, but on the UIApplication iOS APis:
I've found nothing yet to indicate if the app should always call registerForRemoteNotifications or if it can count on isRegisteredForRemoteNotifications and only call it registerForRemoteNotifications when it is false.
I have not either. It's actually called immediately if you're set to auto register for them
react-native-firebase/packages/app/firebase-schema.json
Lines 81 to 83 in 5daa5ff
I'm unaware of who you are as @ourhut or brian@slashtalk.com, but if there is something wrong here, I can certainly correct it
There was a problem hiding this comment.
Thank you for pointing out the place the location the promise resolver and rejecter are referenced, I was looking around and hadn't found it yet. The makes perfect sense.
I also now see why what you've done is correct, and I don't have any cause for concern other than what I got from reading the comment I mentioned.
A style comment that I think would make this clearer, move call to registerForRemoteNotifications into the else block after registering the resolvers, and remove the early return and old comment. I feel like that makes it clear the there are two distinct paths, one that returns TRUE, and one that registers the resolvers and dispatches a request that will resolve/reject them later. Logically the same as what you have now.
Thanks for quickly responding, and I apologize for the noise. This library is amazing, we're getting a lot of value from it and I'm happy to help with improving it as I can.
There was a problem hiding this comment.
Not sure if I'll have time to get to that suggestion right now, but I do agree things could be tidied up there.
Right now I'm still doggedly pursuing other initialization / config-setting changes that cross-cut the whole set of modules, there's quite a few things going on there and some of them have higher priority ecosystem impacts all the way back to firebase-ios-sdk like #4241 which I need to maintain focus on
There was a problem hiding this comment.
Oh, and no worries about noise - I was moving literally as quickly as possible through things over the weekend and no one else has had time to review, so even if the result is the null hypothesis is upheld (no further change necessary) simply having anyone look at it, and even potentially forcing a defense of change, is hugely helpful. I appreciate it
|
Is this fixed already? Or is there some workaround? |
What issue are you running into? |
Even I disabled every auto-init: I am still getting "Access to Local Network" permission popup on iOS which means that messaging is still auto-initialized and calling for additional permissions. |
|
Hi there! I'm not sure the assertion that the "access to local network" popup is connected to this module is correct. I don't believe this module does a local network scan and I thought that was the cause for that popup, if it popped up? Can you point me to some documentation on the apple developer site that shows me what the conditions are for that network popup? As a separate item, you might like this toggle: |
I thought that this: https://rnfirebase.io/reference/messaging#registerDeviceForRemoteMessages is causing permission popup appearance. Maybe I am wrong... Sadly I found just this about the permission popping up on my device when I initialize Firebase: https://www.howtogeek.com/692528/why-iphone-apps-ask-for-devices-on-your-local-network/ |
|
@Baterka I will assert that the "local network permission" request from your app is unrelated to react-native-firebase (or firebase-ios-sdk, our underlying SDK) completely. I use the messaging module. I do not even have the local network permission request plist key entry in my app, and I do not see the popup, and my app works fine. This is the permission https://support.apple.com/en-us/HT211870 Near as I can tell, you've got something else going on in your app. |
Description
Includes a lot of related work, commit by commit, with test support
ios.onlydoes not work,if (device.getPlatform() === 'ios')works)Having read through the current state of ios/android native Info.plist/AndroidManifest.xml data collection settings, we're a fair bit behind with regard to available toggles - I'll try to get the rest in a follow-on PR
Related issues
#4428 asserted that iOS auto registration wasn't correctly handled, but I was unable to reproduce
Release Summary
All conventional commits --> rebase merge
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter