-
Notifications
You must be signed in to change notification settings - Fork 375
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
Add getInitialNotification() on iOS #626
base: main
Are you sure you want to change the base?
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 |
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
onOpened
event 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
onOpened
callback.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