canjs / can-define

Observable properties
https://canjs.com/doc/can-define.html
MIT License
15 stars 7 forks source link

[3.x] Use the single reference for turning off both index and key listeners #456

Closed bmomberger-bitovi closed 5 years ago

bmomberger-bitovi commented 5 years ago

Closes #455

RyanMilligan commented 5 years ago

@bmomberger-bitovi This fix doesn't seem to fully address the issue. It now uses singleReference to look for the handler to remove it, but it never actually stores it to be found. Here's the version I used to solve the issue locally for testing purposes:

    "can.onKeyValue": function(key, handler) {
        var translationHandler;
        var eventKey = key;
        if (isNaN(key)) {
            translationHandler = function(ev, newValue, oldValue) {
                handler(newValue, oldValue);
            };

        }
        else {
            translationHandler = function() {
                handler(this[key]);
            };
            eventKey = 'length';
        }

        singleReference.set(handler, this, translationHandler, eventKey);
        this.addEventListener(eventKey, translationHandler);
    },
    // Called when a property reference is removed
    "can.offKeyValue": function(key, handler) {
        var eventKey = isNaN(key) ? key : 'length';
        var translationHandler = singleReference.getAndDelete(handler, this, eventKey);

        this.removeEventListener(eventKey, translationHandler);
    },
bmomberger-bitovi commented 5 years ago

Thanks for bringing this to my attention, @RyanMilligan . It turns out that when not finding the handler in the single reference, all handlers for the key or index were removed, which is why the test passed. I have a new PR up (#459) that addresses this, as well as making sure that the key "0" is treated as a key, and we should be able to get a new release out tomorrow.