Conversation
|
Travis build failed because the Android build failed, and I didn't change anything on Android side. Can someone please review this PR? |
|
Hi, could you elaborate on this functionality a little bit? |
|
@avishayil well I’m not sure what else can I say to explain what this PR is about... What part don’t you get? |
|
Appearantly your PR documentation appears correctly now for me. Maybe GitHub problems. |
|
This PR is not necessary as the SDK already waits for an observer to be added before it triggers the @habovh if you were adding the Notifications can be opened while the app is already running in the background, so I am also fundamentally opposed to the idea of using a special function to get these notifications. |
|
@Nightsd01 thanks for taking the time to review. If the SDK waits for a listener to be registered, I agree that the race condition scenario I describe is a bug. I wasn't aware that the SDK waits for listener to be registered prior triggering the If the SDK waits as promised, then I understand that |
|
Need this. The OneSignal React-Native iOS SDK is really incomplete compared to the Android one... |
|
This is necessary to handle proper deep links with react-navigation see https://reactnavigation.org/docs/deep-linking/#third-party-integrations. The listener in |
99c2930 to
115b411
Compare
|
Hey there, thanks for submitting this PR. Given how long this has been open, I'm going to close it as part of cleaning up our backlog. |
|
@sherwinski this PR is still relevant and needed. If you feel my time as a contributor is not even valuable enough to give a heads up before closing the PR, I am not going to be opening another one. Feel free to reopen and review this one as part of your backlog cleaning process. |
TL;DR
This PR adds the ability to call
getInitialNotification()on react-native-onesignal.Description
The aim is to provide an API that is similar to React-Native's Linking and PushNotificationIOS modules.
Both of these stock modules allow to register event handlers, as well as providing the initial data that triggered the app launch.
From my point of view, triggering a
onOpenedevent in the case of an app launch from a notification if flawed. You have to race to register your listener before the event is dispatched, and this means you're introducing a luck factor in your app initialization.I cannot rely on luck nor do I wish to race in an already complicated Javascript-timed environment.
So this PR is a neat way of doing things. You can call
getInitialNotification()and get the data from the notification that opened the app. You can call it soon, late, it doesn't matter, it's available 24/7.It also comes in handy in case you need to store the initial notification or need to deal with it at a later point. No more redux needed to save your notification for later use.
This addresses #332, for real this time.
Android notice
I'm no Android developer, and I did not implement the function in the native Android module. If someone feels like Android could benefit from this as well, don't hesitate to contribute!
Trying to call
getInitialNotification()on Android will simply console.log a message saying it's iOS only.Usage
Breaking changes
If the iOS app is opened by a notification, the native module will no longer trigger the
onOpenedcallback.This choice has been made to prevent cases where the old way was working [insert random number here]% of the time. A working event trigger AND using
getInitialNotification()could make you handle the initial notification twice. So we're avoiding this right of the bat.You can easily update your code by adding
getInitialNotification()in addition to your listener, and basically give it the same callback. This ensures you handle the initial notification properly, and your listener handles any future notifications.This change is