b8ne / react-native-pusher-push-notifications

Manage pusher interest subscriptions and notification events in Javascript.
MIT License
97 stars 85 forks source link

Adding an "off" to remove eventlisteners. #27

Closed emilkarl closed 5 years ago

emilkarl commented 5 years ago

Any reason there isnt any off method to remove eventlisteners. I am thinking of notification specifically.

Lets say you login and allow notifications, then in the app we subscribe to the interests and registers a handleNotification listener. When the user logs out we ubsubscribe interests but do not remove the listener in some way?

I've been seeing multiple events be triggered in my handler after a while when using the app. Any ideas?

https://github.com/b8ne/react-native-pusher-push-notifications/blob/286e506d40574190cebdbdb0fccbf6d4feadda88/index.js#L54

emilkarl commented 5 years ago

Seems to be that the notification is sent twice. Once for background notification and when for the click. But in the JS code, we have no idea which one it is? Any ideas if we could add this easily seems to be available in other libs. @b8ne @lukabratos

b8ne commented 5 years ago

Hmm yeah an off method should be there, and wont be too hard to implement. I also think we should implement the unsubscribeAll method, which I think you initially had, but was throwing errors for me so removed it temporarily to get the core right. In regards to background notifications, I've been working on this too. Im not getting dupes, but have noticed that its hard to distinguish between when:

  1. App is closed and notification clicked
  2. App is in background and notification clicked
  3. App is in foreground

I've seen a few examples that utilise PushNotificationIOS but we obviously dont want to go adding more packages. Will keep working on it.

emilkarl commented 5 years ago

Ok, cool. I think it also could depend on the content-available=1 which for me is always set by Pusher. I don't add it to my call to Pusher them the notification I get to the phone has that flag set.

Here is an article describing some behavior of this, a bit old but seems legit: https://samwize.com/2015/08/07/how-to-handle-remote-notification-with-background-mode-enabled/

b8ne commented 5 years ago

@lukabratos @emilkarl i think i've found a pretty simple solution. I tried a few 3rd party packages and they all seemed very bulky. Looking into app states, i found that for iOS at least, there are 3 states we need to worry about:

  1. UIApplicationStateActive: app is in foreground
  2. UIApplicationStateInactive: app is dead, not opened at all
  3. UIApplicationStateBackground: app is opened in background

I also found that if a notification is clicked when the app is in the background, didReceiveRemoteNotification fires twice for both UIApplicationStateInactive and UIApplicationStateBackground states. Given we can really treat cases 2 and 3 above as the same, by filtering out all UIApplicationStateBackground events we can get a valid app state that only fires notifications once.

- (void)handleNotification:(NSDictionary *)notification
{
    RCTLogInfo(@"handleNotification: %@", notification);

    UIApplicationState state = [UIApplication sharedApplication].applicationState;

    if (state == UIApplicationStateBackground) {
        // Prevent background state firing twice
        // stackoverflow.com/questions/20569201/remote-notification-method-called-twice
        return;
    }

    [RNPusherEventHelper emitEventWithName:@"notification" andPayload:@{
                                                                        @"notification":notification,
                                                                        @"background":[NSNumber numberWithBool:state != UIApplicationStateActive]
                                                                        }];
    [[PushNotifications shared] handleNotificationWithUserInfo:notification];
}

Try that out and let me know how it goes.

emilkarl commented 5 years ago

@b8ne I'm currently investigating this. I think you want to trigger it in the background as well...but you want to be able to handle the event differently than a click. I will test this and see how it goes.

emilkarl commented 5 years ago

Do you want to test this? Think it's solved now with #28 @b8ne @ewal @lukabratos

b8ne commented 5 years ago

@emilkarl looks good, thanks mate.