[BUGFIX beta] Observers after Dependent Key Change

[mmpestorich]

Currently when a property is overriden in an extended class and its
dependent keys change, observers watching that property will still fire for
changes made to the old set of dependent keys.

For example (http://emberjs.jsbin.com/nujofu/1/edit?html,js,output):

var count = 0;

var MyClass = Ember.Object.extend({
    prop1: 'foo',
    content: Ember.computed.alias('prop1'),
    incrementCount: Ember.observer('content', function () { count++; })
});

var obj = MyClass.extend({
    prop2: Ember.computed.alias('prop1'),
    content: Ember.computed.alias('prop2') // redefine content with diff depKey
}).create();

Ember.run(function() {
    obj.set('prop1', 'bar');
});

count === 1; // Should be 1; but in fact is 2

The problem is that iterDeps will loop over deps that no longer exist and invoke prop will/did change methods.

[rwjblue]

In general, the test case that is passing seems good to me, but I think this will need @stefanpenner and/or @krisselden to review in detail....

[stefanpenner]

these counts exist for a reason, i don't believe we can binary enable and disable these without risking unsubscription for other listeners

[stefanpenner]

something feels fishy

[mmpestorich]

Initially that's what I thought but I couldn't find a single test that illustrated that. After tracing the code I can't see how that count could ever exceed 1. meta.deps is incremented and decremented when properties are setup and torn down at the time they're defined and no where else that I could find. keyName is the name of that property being defined and depKey is the name of a property that keyName depends on (the comment deleted above is wrong; depKey here doesn't depend on keyName... rather its the other way around). So how can you have a property that depends on the same key more than once? ... of course I could be missing something but that's what I took away from it earlier

[stefanpenner]

i'll take some soon to do a thorough investigation. Little low on time right now.

[mmpestorich]

I made a couple more small changes and added another test so that redefining a property with dependent keys on the same object (versus a subclass) will properly fire property change events as well. Also because meta.deps is an inherited hierarchy of dependent keys for given key names it's not always safe to delete the keyName for a given depKey in removeDependentKey. I set it to false instead and use a guard in iterDeps to ensure disabled depKeys don't trigger change behavior (I still believe that it is safe to handle them in a binary fashion).

[rwjblue]

@stefanpenner - I'm gonna go ahead and assign this one to you since you mentioned you would like to dig into it a bit further before merging.

[stefanpenner]

I'm gonna go ahead and assign this one to you since you mentioned you would like to dig into it a bit further before merging

👍

[krisselden]

@stefanpenner if these test fail without this change, that would be very serious, it used to be that defineProperty() caused teardown if there was existing keys. I'm concerned that isn't the case anymore.

The counts seem to be handling the case of a dependent key being listed twice which seems weird. I'm not sure if that is important, but I would like to get these tests passing ASAP.

[krisselden]

@mmpestorich it is simpler fix to just have iterDeps check the ref count, at some point during the various refactorings, iterDeps actually lost the guard that checked this. Most likely because of the reuse of a variable name that made it look like a redundant check.

[krisselden]

this check is enough to pass the tests without the above no?

[mmpestorich]

@krisselden Yes, that guard is enough to pass the tests... I'd be happy to remove the other if that will get this merged quicker, just let me know... (though I would still like to know the reason that the code allows for the same dependent key multiple times... doesn't make sense to me).

[rwjblue]

@mmpestorich - I'd happily merge the test + guard here.

I would still like to know the reason that the code allows for the same dependent key multiple times... doesn't make sense to me

Take a look at http://numberofpeoplewhounderstandembermetal.com. It doesn't make sense to me either, but lets fix your failure scenario first...

[mmpestorich]

@rwjblue updated to include just the guard + tests

[rwjblue]

Thank you!