Regression from v2.1.1 to 2.2.0

[kfiroo]

Hey! Any major, possibly breaking, changes made to android notifications? I stopped receiving notifications after upgrading to 2.2.0. Can't see any errors in adb logcat.

When reverting back to 2.1.1 I get the notifications again :(

@npomfret @zo0r any idea? lead?

Thanks

[npomfret]

I thin the last release was pretty major, but I can't remember any change that would have broken an existing setup.

Are you seeing any messages in logcat relating to RNPushNotification?

[kfiroo]

Yeah, I know last release was massive :) Unfortunately, logcat doesn't provide any info when receiving notifications on both versions. I guess I'll start debugging

[kfiroo]

@npomfret Looks like with v2.2.0 the notification reaches the JS code, but no notification is being presented by the OS. Any clue?

[npomfret]

Logcat should at least show stuff when the app starts though - do you not see anything? If not it's probably not linked correctly. Make sure you don't have anything filtered and set check debug level log lines

[kfiroo]

@npomfret Yeah, I mean it shows nothing relevant

11-07 13:08:13.236  2598  2598 I RNPushNotification: RNPushNotificationBootEventReceiver loading scheduled notifications
11-07 13:09:23.262  2922  2973 I RNPushNotification: Cancelling all notifications
11-07 13:09:23.262  2922  2973 I RNPushNotification: Clearing alerts from the notification centre
11-07 13:09:52.241  2922  3056 I RNPushNotification: Cancelling all notifications
11-07 13:09:52.241  2922  3056 I RNPushNotification: Clearing alerts from the notification centre
11-07 13:14:58.796  3296  3344 I RNPushNotification: Cancelling all notifications
11-07 13:14:58.796  3296  3344 I RNPushNotification: Clearing alerts from the notification centre
11-07 13:25:57.393  3580  3629 I RNPushNotification: Cancelling all notifications
11-07 13:25:57.393  3580  3629 I RNPushNotification: Clearing alerts from the notification centre

[kfiroo]

@npomfret I might be missing something but looks like RNPushNotificationListenerService.sendNotification only calls RNPushNotificationJsDelivery.notifyNotification.

I'm missing the call to RNPushNotificationHelper.sendNotification to actually present the notification.

Could it have been removed in this commit RNPushNotificationListenerService.java#L108

private void sendNotification(ReactApplicationContext context, Bundle bundle) {

    // If notification ID is not provided by the user for push notification, generate one at random
    if (bundle.getString("id") == null) {
        Random randomNumberGenerator = new Random(System.currentTimeMillis());
        bundle.putString("id", String.valueOf(randomNumberGenerator.nextInt()));
    }

    Boolean isRunning = isApplicationRunning();

    RNPushNotificationJsDelivery jsDelivery = new RNPushNotificationJsDelivery(context);
    bundle.putBoolean("foreground", isRunning);
    bundle.putBoolean("userInteraction", false);
    jsDelivery.notifyNotification(bundle);

    // If contentAvailable is set to true, then send out a remote fetch event
    if (bundle.getString("contentAvailable", "false").equalsIgnoreCase("true")) {
        jsDelivery.notifyRemoteFetch(bundle);
    }
}

[npomfret]

I'm not sure, quite possibly. maybe @mikelambert can help?

I don't really understand how it's working for me without this call. I have both local and remote push notifications working fine with RN0.36 and 2.2.0. Is it possibly an NPM issue? How does the code on your disk compare to what's in master?

[kfiroo]

@npomfret @mikelambert Changing RNPushNotificationListenerService.sendNotification to the following seems to fix the issue, what do you think?

private void sendNotification(ReactApplicationContext context, Bundle bundle) {

    // If notification ID is not provided by the user for push notification, generate one at random
    if (bundle.getString("id") == null) {
        Random randomNumberGenerator = new Random(System.currentTimeMillis());
        bundle.putString("id", String.valueOf(randomNumberGenerator.nextInt()));
    }

    Boolean isRunning = isApplicationRunning();

    RNPushNotificationJsDelivery jsDelivery = new RNPushNotificationJsDelivery(context);
    bundle.putBoolean("foreground", isRunning);
    bundle.putBoolean("userInteraction", false);
    jsDelivery.notifyNotification(bundle);

    // If contentAvailable is set to true, then send out a remote fetch event
    if (bundle.getString("contentAvailable", "false").equalsIgnoreCase("true")) {
        jsDelivery.notifyRemoteFetch(bundle);
    }

    // Call RNPushNotificationHelper.sendNotification here to present the notification if app is not running
    if (!isRunning) {
        Application applicationContext = (Application) context.getApplicationContext();
        RNPushNotificationHelper pushNotificationHelper = new RNPushNotificationHelper(applicationContext);
        pushNotificationHelper.sendNotification(bundle);
    }
}

[npomfret]

I'd really like to get to the bottom of it before we make any changes. Hopefully @mikelambert can advise. Mine is working fine, it makes no sense that your's isn't. Although there's quite a few people on here having issues with their android installation - maybe this is why?

If I do this:

    onNotification(notification) {
      console.log("Received pushNotification", notification);
    }

I see this:

'Received pushNotification', { foreground: false,
  'google.sent_time': 1478523203864,
  payload: '{"debug":{"messageText":"\'Sd\'"},"sender":"testaccount 88","delivery_receipt_id":"b6d6e459-4603-4ff9-aefe-53f1ac2d0799","id":"c0e1f176-41d2-460d-a4a3-707f7135409c","notification_id":"c0e1f176-41d2-460d-a4a3-707f7135409c","chat_group_id":"ed401c65-b287-44fc-8ece-c8f0c167a3e8","object_id":"af65d27f-039a-4b64-859b-b9a50581d39e"}',
  userInteraction: false,
  id: '1631144392',
  'google.message_id': '...',
  collapse_key: 'new_message' }

I wonder if it's anything to do with the content? My app receives 'silent' push notifications with a payload which are not intended for the notification centre. They get delivered to the app, which process them (downloads some content) and then fires a local notification to alert the user. What do the notifications you're sending look like?

[npomfret]

Ah, yes. That's exactly the problem... i think. So the new release passes remote push notifications to the JS code, but not to the notification centre. @mikelambert was this intentional?

[kfiroo]

@npomfret OK, I think I got it. You are seeing the notification in your JS code, I get the same behaviour, but as you can see in the code snippet above, the code in RNPushNotificationListenerService.sendNotification calls RNPushNotificationJsDelivery.notifyNotification which sends the notification data ONLY to the JS app.

The problem is that when the app is not running I would like the notification to be presented to the user, as it used to work before, I don't want to create it myself with a local notification (although I might have to do it now)

What I understand from you is that it doesn't work for you either, remote notifications are not shown, and you bypass that by issuing a local notification from the JS code, right?

[npomfret]

I think so. So it sounds like we've lost the ability to send a single notification to the app as well as the notification center. I don't know if this was an intentional change though. Need to wait for mike to respond... but I think you're fix should resolve it if not. Could you test it in your fork?

The other work around is that this project will now actually start your app if it isn't running and deliver the silent notification. Which means you can always present something to the user in the notification centre. You just need to structure you app in such a way that the push notification code can run without the react-native life-cycle stuff kicking in. This is what I've done and it works well.

[kfiroo]

@npomfret Not sure this is the right direction for the project, remote notifications in the notification centre are a trivial part of notifications handling, see PushNotificationIOS for example.

I've tested my fork, looks like it's working fine. I'm pushing it to production until this issue is resolved.

I submitted a PR with this fix to be accepted or rejected https://github.com/zo0r/react-native-push-notification/pull/269

[cinder92]

hello guys! it seems method onRegister is never firing in both, ios and android, i need the token, to send push notifications.

any update about this?

[mikelambert]

Hey sorry about the breakage. :(

My goal was to make the RN-PN handle notifications identically, regardless of whether the app was open or closed. It does this by starting the app (without UI), to deliver the notification to JS. @kfiroo , are you suggesting this is a wrong direction to go?

I did not intend to make a breaking change, or stop presenting remote notifications when they are received. I do totally agree this buggy behavior is the wrong direction for the project. :P

I believe the correct fix is to delivering the remote notification to the notification center and the JS code:

• If it's presentable notification, it will display. (I didn't like this, since it was an un-localizable message.) The JS code can ignore this notification if it wants. This is @kfiroo 's case.
• If it's not, the client JS code will handle it and present a new local notification properly. This is my and @npomfret 's case.

I am a bit unclear on @kfiroo 's proposed patch, however. It seems to present the received notification to the notification center only when the app is not currently running. I'm curious, what is the old behavior when a remote notification is received while the app is running. Will it not be sent to the notification center, and only be sent to JS? (It seems complex...but maybe that's because most apps will want to display that using in-app UI?)

[sagark1510]

How to receive notification on JS thread when application is totally closed?? I have added code under root component's componentWillMount event and its not working.

[mikelambert]

@sagark1510 This thread is not the place to ask for help using this feature.

If you want that, please go read the thread where the feature was added: https://github.com/zo0r/react-native-push-notification/pull/220 (Though I suspect we'll need to add better documentation of this feature when the dust settles...)

[npomfret]

"It seems to present the received notification to the notification center only when the app is not currently running"

Is supposed to mimic the iOS behaviour where the notification only appears if the app is running in the background?

Also (and slightly unrelated), I noticed a weird check for a content-available flag. I think this can be removed, it's an iOS APNS thing and doesn't need to be in the android code.

[kfiroo]

@mikelambert @npomfret Hey! I absolutely agree with your approach on handling notifications the same way when the app is opened or closed, the headless JS task is awesome! Great job! :)

That being said, I strongly believe the direction this project should go is to provide an android implementation of PushNotificationIOS. Unless we want to write our own implementation of the IOS code, we should try to avoid drifting too far from it.

For example, it makes no sense to call the JS code when a notification arrives only on android.

Currently, PushNotificationIOS shows a notification in the notification center when the app is closed, and I don't think it calls the JS code (not sure though).

What do you guys think?

[npomfret]

The apple notification only goes to the notification centre if the app is in the background AND the notification is a 'noisy' notification, i.e. there is something to show (as opposed to a silent notification with content-available=1 that has nothing to show).

I think adding the line back in that sends the notification to the notification centre if the app is running in the background would please people. Seems harmless enough.

[npomfret]

@mikelambert do you feel like you could make a PR to revert the regression?

[mikelambert]

@kfiroo , my intent was to support PushNotificationIOS eventually through the use of a content-available=1 hack. I haven't actually integrated notifications into my iOS app yet though, so haven't bothered to attempt this feature at all. :/ It's possible that it might be impossible to implement in an API-compatible way on iOS, and I'm not sure what we'd want to do at that point.

@npomfret , I was trying to understand the behavior of @kiroo 's patch above. Particularly to verify the old behavior of push-notifications on android, that it's actually as complex as it sounds, to verify that this patch returns to the right level of complexity. If I were to attempt it, based on my current understanding, it'd basically be the same as @kfiroo 's change.

[kfiroo]

My gut feeling about react-native libraries is that iOS and Android should behave as close together as possible, when the API is the same (I know it's not always that simple).

@mikelambert I understand that iOS might not be in your concern right now, so it makes sense you'd want to make the android version better, but the cost is that we diverge from PushNotificationIOS and I'm not sure it's worth it.

I feel like my PR is a good compromise between the 2 approaches.

@npomfret @mikelambert @zo0r As for further commits in this project I think it would be wise to keep PushNotificationIOS in mind when changing the core behaviour for the community's sake :)

[npomfret]

I agree that it's a sensible goal to keep the behaviours the same where appropriate. And here I think it is appropriate.

We can't make them exactly the same because there are some fundamental differences in the OS. For example, currently if the user kills their Android app it will get relaunched when a push notification arrives (if you've set your manifest up correctly). On iOS this won't happen, the user must restart the app manually if they killed it. However, if iOS choses to kill the app (which it does, quite aggressively) then it will also restart your app should a push remote notification arrive with content-available=1 - this is already the case.

I think we're currently debating what should happen if a push notification arrives that is both silent and noisy. By silent I mean that one that has a payload which needs to be delivered to the JS code, and by noisy I mean it has visual content that needs to be displayed in the notification centre (text, sounds vibrations etc).

I didn't notice this break when testing @mikelambert's change because my app doesn't mix the 2 types of notification. They're either noisy or silent but not both. But mixing them is a valid thing to do... I think. Sorry about that.

I understand (I hope!) that in iOS, if you get a mixed notification it will only appear in the notification centre if the app is running in the background. It will not display the notification centre if the app is in the foreground. And a mixed notification will be delivered to the JS code regardless of whether or not it's running in the background or foreground in order that it can deliver the payload, just like a regular silent notification.

[mikelambert]

FWIW I have done more investigation, and am fine with @kfiroo 's proposed changelist to fix the problems people are seeing (though had a preference for some renaming to clarify things).

Yes, there is still a "larger" conversation to be had about Android/iOS compatibility, the use of content-available=1 on iOS and what that means for the details of an equivalence on Android.

[mikelambert]

Can this be closed now, with https://github.com/zo0r/react-native-push-notification/pull/269 having been committed and released in 2.2.1?

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.