chore: Use main thread for requesting notification permissions. #90
Conversation
iOS throws a warning that registerForRemoteNotifications must be called from the main thread only, this just wraps that call in a block to ensure it always runs there, regardless of how the containing function is called.
kylekurz
changed the title
|
Ping on this... |
|
Ping this. I'm waiting for this fix. |
|
How come this is not merged to the code base??? I mean this is such a big improvement over what we had previously. In case of newer devices without the main thread resolution we won't be able to receive any notifications. Where is the library maintainer and what is he doing? This is already 2 weeks for a fix that is so essential and obvious to check that it makes my eyes bleed! |
|
In defence of the maintainer, this is an open source project. Nobody pays for work on this. If it's critical for your app you can always use push-notificaitons-ios from your fork and change to upstream when it's merged. |
| [RCTSharedApplication() registerForRemoteNotifications]; | ||
| dispatch_async(dispatch_get_main_queue(), ^(void){ | ||
| [RCTSharedApplication() registerForRemoteNotifications]; | ||
| }); | ||
| [UNUserNotificationCenter.currentNotificationCenter getNotificationSettingsWithCompletionHandler:^(UNNotificationSettings * _Nonnull settings) { |
There was a problem hiding this comment.
You should run the getNotificationSettingsWithCompletionHandler: call into Main Thread as well.
There was a problem hiding this comment.
My goal was to change the minimum amount to clear Apple's warnings. They do not show a warning about calling that function from another thread, so I didn't change it. This PR is simply to clean up Main Thread warnings, so this suggested addition is out of scope.
There was a problem hiding this comment.
Yep but as you get it this code was running in background thread so the getNotificationSettingsWithCompletionHandler: will be too even if you didn't get warning by Apple is good pratice in iOS to register observer in main thread.
By experience we faced issues where when you don't register on main thread your observer completion was never fired.
There was a problem hiding this comment.
I disagree. You shouldn't be doing more work on the Main (UI) thread than you have to. If Apple is happy to let you grab those settings from a non-UI thread, you should do so. The Main thread should be reserved for actions that require user interaction or drawing of elements.
|
Sorry for the late response! |
|
Well if you get the callback fine.
|
Naturalclar
changed the title
|
Published v1.1.1 |
Summary
iOS throws a warning that registerForRemoteNotifications must be called
from the main thread only, this just wraps that call in a block to
ensure it always runs there, regardless of how the containing function
is called.
Test Plan
Ran on a device under debug and saw the thread safety warning go away
What's required for testing (prerequisites)?
Physical device
What are the steps to reproduce (after prerequisites)?
Call the method below from a thread other than the main UI thread:
RCT_EXPORT_METHOD(requestPermissions:(NSDictionary *)permissions resolver:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject)Compatibility
Checklist
README.mdCHANGELOG.mdexample/App.js)