cbcrc / koco-router

Simple Knockout router
MIT License
2 stars 1 forks source link

unsub issue #8

Closed ddamico-cbc closed 9 years ago

ddamico-cbc commented 9 years ago

@th3answer and I have been investigating a bug where an edit page (an instance of the content-edit-page-base type) that a user has edited does not perform correct "edited form" behaviour (confirm before navigating away) if you arrived at it by navigating from another instance of the content-edit-page-base type.

After a lot of headscratching, what we found was that when you are moving from one of these edit page instances to another, the new edit page is being removed from the router's event subscriber list because the remove is only comparing the identity of the subscriber handler, which is a reference to the base's prototype, which the two pages share. Here's the code in question:

RouterEvent.prototype.unsubscribe = function(handler) {
    var self = this;

    _.remove(self.subscribers, function(subscriber) {
        return subscriber.handler === handler;
    });
};

Our proposed workaround is to pass, as we do in subscribe, both the handler AND the context, and then compare both instead of just the handler. Like so:

RouterEvent.prototype.unsubscribe = function(handler, context) {
    var self = this;

    _.remove(self.subscribers, function(subscriber) {
        return subscriber.context === context && subscriber.handler === handler;
    });
};

Downsides to doing this... it's a breaking change, and it may not capture the original intent. Upsides... it works!

We are very open to alternate suggestions on this if you think there's a better way to get to same goal, but it does seem like having multiple instances of the same base type subscribed to router events at the same time is something we need.

W3Max commented 9 years ago

I checked and I think that your proposed solution is good. Do you think we could do the subscriber.context === context only when there is 2 args passed to the unsubscribe function so that we do not create a breaking change?

ddamico-cbc commented 9 years ago

Thanks for your help with this Max, I've made the change in release 0.4.2.