OneSignal / react-native-onesignal

React Native Library for OneSignal Push Notifications Service
Other
1.57k stars 373 forks source link

[iOS] Crash on iOS 9.3.2 because of missing didReceiveRemoteNotification method #259

Closed negativetwelve closed 6 years ago

negativetwelve commented 7 years ago

In the installation instructions, it says to add:

// Required for the notification event.
- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)notification {
    [RCTOneSignal didReceiveRemoteNotification:notification];
}

to the AppDelegate. This line is causing a crash anytime we receive a push notification, but only on devices that aren't iOS 10. This is using the latest (3.0.4) react-native-onesignal library.

Removing this method still allows the device (and OneSignal) to receive push notifications, but the crash now does not occur.

It looks like this method was removed in this commit: https://github.com/geektimecoil/react-native-onesignal/commit/4b2c55da780cff9bc82c227c052b416681dfdb27#diff-53b840050097cad618dd73cd2ebd59f2L81 and the previous comment makes it seem like it's no longer required.

Unfortunately, it's still in the header file (https://github.com/geektimecoil/react-native-onesignal/blob/master/ios/RCTOneSignal/RCTOneSignal.h#L17) so our app compiled and then caused a runtime error when it could not find the selector :(

Couple of questions:

  1. Is it okay to remove this method? Are we missing functionality by removing it?
  2. If it is okay to remove, should the method definition in the header be removed and the readme updated? If so, I would love to submit a PR for that.
zakster12 commented 7 years ago

I have the exact same problem. screen shot 2017-05-23 at 12 39 16 pm

screen shot 2017-05-23 at 12 40 34 pm

zakster12 commented 7 years ago

@negativetwelve I removed the

- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)notification {
    [RCTOneSignal didReceiveRemoteNotification:notification];
}

and everything works just fine. It wasn't necessary to do anything else but I am also interested in this question:

Are we missing functionality by removing it?

negativetwelve commented 7 years ago

@zakster12 sorry for the late reply. I haven't worked on purely native in a while so I was unsure if that hook was necessary for anything. I think I remember seeing that the native OneSignal SDK is now hooking into this method on its own which means that the need to add it manual is no longer required (hence why they removed it)

negativetwelve commented 7 years ago

it's been working fine for us on iOS 9 with that method removed and we're indeed still receiving push notifications with the proper OneSignal JS hooks!

zakster12 commented 7 years ago

@negativetwelve did you receive notifications when the app is killed/closed? Because I do not receive them: I have content available set to true and

onReceived = (receivedNotif) => {
}

but I receive them only the app is not closed. Can you help here?

negativetwelve commented 7 years ago

hi @zakster12, we currently do not take advantage of the content-available for notifications but our devices are all receiving push notifications when the app has been force closed (not in memory)

jemmyphan commented 6 years ago

@negativetwelve does it work when the app is active, mine didn't work when it is active ( it works while the app is in background)

negativetwelve commented 6 years ago

@jemmyphan sorry, this all happened a while ago and we've since updated versions of the OneSignal SDK

jemmyphan commented 6 years ago

@negativetwelve nvm, thanks anyway.

ghost commented 6 years ago

Thanks very much @negativetwelve

That solved my problem - I followed the guide over on Medium https://medium.com/differential/react-native-push-notifications-with-onesignal-9db6a7d75e1e which was incredibly helpful, it saved me a tonne of time - I'll add a comment on there to report of this, looking at the guide it seems that Pod isn't required either now.

Thanks for your help!