register -> unregister lifecycle

[fredgalvao]

I'm updating my project from 1.2.3 to 1.4.4, and I realized that we can do better than the .off() functions.

I just confirmed that the following end up duplicating all listeners on the _handlers object:

class PushNotificationService {
    api;
    constructor() {
        this.api = PushNotification.init({...});
        this.register();
    }

    register() {
        this.api.on('registration', () => {...});
        this.api.on('notification', () => {...});
        this.api.on('error', () => {...});

        something.register.at.my.back.end();
    }

    unregister() {
        this.api.unregister(...);

        something.unregister.at.my.back.end();
    }
}

class PushNotificationController {
    isItOn: boolean = true;
    toggle() {
        this.isItOn = !this.isItOn;

        if (this.isItOn) {
            pushNotificationService.register();
        } else {
            pushNotificationService.unregister();
        }
    }
}

<button ng-click='toggle()'>Do it</button>

The way I'm doing it at the moment, I don't .off() the listeners (simply because I didn't have these functions up to now, 1.2.3 didn't have them), so when I toggle twice, it'll end up being registered but with 2 listeners for each event type. And then I wondered, shouldn't .unregister() automatically .off() all listeners? Either way, we should/could improve the docs on this so that a suggested way to do it exists. I'd say "toggle push notification reception" is a pretty common feature to have in apps.

Options

Automatically:

PushNotification.prototype.unregister = function(successCallback, errorCallback, options) {
    if (errorCallback == null) { errorCallback = function() {}}

    if (typeof errorCallback != "function")  {
        console.log("PushNotification.unregister failure: failure parameter not a function");
        return
    }

    if (typeof successCallback != "function") {
        console.log("PushNotification.unregister failure: success callback parameter must be a function");
        return
    }

    var cleanHandlersAndPassThrough = function() {
        this._handlers = {
            'registration': [],
            'notification': [],
            'error': []
        };

        successCallback();
    }

    exec(cleanHandlersAndPassThrough, errorCallback, "PushNotification", "unregister", [options]);
};

Suggest user to deal with it:

README.md <

If you want to unregister, make sure you clean up your handlers using `.off()` on all events.

[macdonst]

@fredgalvao Yeah, you are right. I that your approach of automatically removing the handlers when doing an unregister is the way to go. Please go ahead and make the change to the JS.

[macdonst]

@fredgalvao wrote this in a car without internet, trying to stay productive. Please review when you have a few moments.

[fredgalvao]

As far as I can tell, it's enough. My point when creating the issue (that I dumbly forgot to mention before) was to ask you: Is there anything that needs to be done in native code to make sure we actually cleaned up these listeners?

If the answer to that is no, then I'm fine and we can rest in pieces.

[macdonst]

Nope, there is nothing on the native side that needs to be done for these handlers to be cleared. I had to ride in a car for an hour today so I ended up writing the test cases for this feature then using your code so well done.

[lock]

This thread has been automatically locked.