Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

habovh
Copy link

@habovh habovh commented Sep 10, 2018

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

import OneSignal from 'react-native-onesignal'

OneSignal.getInitialNotification().then((notification) => {
this.onNotificationOpened(notification)
}).catch((error) => { console.error(error) })

// or

OneSignal.getInitialNotification().then((notification) => {
  this.onNotificationOpened(notification)
})

// or

try {
  const notification = await OneSignal.getInitialNotification()
  console.log(notification)
} catch (error) {
  console.error(error)
}

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 Reviewable

@habovh
Copy link
Author

habovh commented Sep 17, 2018

Travis build failed because the Android build failed, and I didn't change anything on Android side. Can someone please review this PR?

@avishayil
Copy link
Contributor

Hi, could you elaborate on this functionality a little bit?

@habovh
Copy link
Author

habovh commented Jan 6, 2019

@avishayil well I’m not sure what else can I say to explain what this PR is about... What part don’t you get?

@avishayil
Copy link
Contributor

Appearantly your PR documentation appears correctly now for me. Maybe GitHub problems.
@jkasten2 why was this hadn't been approved yet?

@Nightsd01
Copy link
Contributor

Nightsd01 commented Jan 6, 2019

This PR is not necessary as the SDK already waits for an observer to be added before it triggers the opened event. The race condition scenario described in the initial PR should not be a concern for this reason.

@habovh if you were adding the opened observer yet weren’t getting all opened events, that would definitely be a bug. We’ve actually fixed a few bugs related to this in recent versions of the SDK so you may want to check.

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.

@habovh
Copy link
Author

habovh commented Jan 7, 2019

@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 opened event, mainly because it simply didn't work in my case.

If the SDK waits as promised, then I understand that getInitialNotification() should not be used to emulate the same behaviour, hence making my PR irrelevant.
By the way, my PR didn't prevent the already-running app receive and handle notifications using the opened event!
However, it then could be useful to have this getInitialNotification() available in order to be able to check if the app was opened from a notification, and to recall the data associated with the said notification programatically at any given time. Just an idea here.

@nmateo
Copy link

nmateo commented Mar 25, 2024

Need this. The OneSignal React-Native iOS SDK is really incomplete compared to the Android one...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants