mikehostetler / amplify

AmplifyJS
http://amplifyjs.com
GNU General Public License v2.0
1.45k stars 144 forks source link

Bug in unsubscribe with equal callbacks #85

Closed redmagic closed 11 years ago

redmagic commented 11 years ago

Hi,

When 2 subscribers are listening to a topic and have the same callback function, but are registered with different contexts, the unsubscribe will fail. The first listener will be unsubscribed.

Example:

function A() {

    amplify.subscribe('myevent', this, this.doIt)
}
A.prototype = {
    doIt: function () {
        .....
    }
}
var x = new A(), y = new A();

amplify.unsubscribe('myevent', y, y.doIt); //Will fail

Fix:

Check for context equality on unsubscribing:

        for ( ; i < length; i++ ) {
            if ( subscriptions[ topic ][ i ].callback === callback && subscriptions[ topic ][ i ].context === context ) {
                subscriptions[ topic ].splice( i, 1 );
                break;
            }
        }
dcneiner commented 11 years ago

Hi @redmagic, I worked on a new test to try to duplicate your issue. It seems to unsubscribe correctly with the current functionality.

amplify.unsubscribe('myevent', y, y.doIt); //Will fail

That will certainly fail because you cannot pass in the context to the unsubscribe method currently. However, this works as expected:

amplify.unsubscribe( 'myevent', y.doIt )
dcneiner commented 11 years ago

@redmagic – Ah, nevermind, my test was not fully showing the problem. You are correct this is a bug.

dcneiner commented 11 years ago

@redmagic - Please download this version and see if it fixes your issues. Thanks for alerting us to the issue. I also adjusted the behavior for unsubscribe to unsubscribe all matching functions (and matching context if provided) instead of just the first one.

redmagic commented 11 years ago

Hi Douglas,

Thanks for your quick response to this issue. I've tested the new build and it resolves the issue correctly.

Keep up the good work!