-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[BUGFIX beta] Observers after Dependent Key Change#11550
[BUGFIX beta] Observers after Dependent Key Change#11550rwjblue merged 1 commit intoemberjs:masterfrom
Conversation
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 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.
3aab493 to
55d3109
Compare
|
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.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these counts exist for a reason, i don't believe we can binary enable and disable these without risking unsubscription for other listeners
|
something feels fishy |
|
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 |
|
i'll take some soon to do a thorough investigation. Little low on time right now. |
55d3109 to
2eb5730
Compare
|
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 |
|
@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 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. |
|
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is enough to pass the tests without the above no?
|
@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). |
|
@mmpestorich - I'd happily merge the test + guard here.
Take a look at http://numberofpeoplewhounderstandembermetal.com. It doesn't make sense to me either, but lets fix your failure scenario first... |
dependent keys change, observers watching that property will still fire
for changes made to the old set of dependent keys.
```js
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');
});
// assert(count === 1); // Fails, should be 1 but in fact is 2
```
2eb5730 to
75a7ba3
Compare
|
@rwjblue updated to include just the guard + tests |
[BUGFIX beta] Observers after Dependent Key Change
|
Thank you! |