Fix for pr 12215 #12221
Fix for pr 12215 #12221
Conversation
stefanpenner
commented
Aug 27, 2015
stefanpenner
commented
- actually use DK's instead of observers, and everything becomes nicer.
- @krisselden r?
- awesome diagram only useful to me:P
- add test for recursive case
- add test for crazy case with a mix of async and sync
- add instance DKs
stefanpenner
force-pushed
the
fix-for-pr-12215
branch
from
8da4e89
to
fc0cc4a
Compare
|
looking at these existing code, it is very likely to leak observers. |
|
I think i'll take a stab at instead of using observers i'll instead dynamically add dks addDepedentKey I just want it to use a more internally public API, rather then just pushing onto the dpKeys // time for dinner, will look again in the AM. |
Yea that's what I was worried about. Since there really is no formal destruction for this computed property. Thanks again for the priority :) |
yup, but pushing it to DK's "just fixes it" so i'll do that in the AM |
stefanpenner
force-pushed
the
fix-for-pr-12215
branch
2 times, most recently
from
2e5dbc9
to
c316c36
Compare
|
Alright, I am actually relatively pleased with the solution. It would be great to make the dynamic DK stuff public API somehow. @krisselden this one needs your eyes. |
|
|
||
| cp._sortPropObservers = []; | ||
| if (sortDependentKeys) { | ||
| remove(cp._dependentKeys, sortDependentKeys); |
There was a problem hiding this comment.
can this be an method of CP? I'm hoping to reduce bookkeeping meta stuff from being sprinkled around thing, also CP descs are shared between instances, is this sym specific to this instance?
There was a problem hiding this comment.
CP descs are shared between instances, is this sym specific to this instance?
ya this will fail. I'll need to bring back the weakmap
There was a problem hiding this comment.
Actually, I don't believe this is actually an issue. Since DK changes are only taken into account after the CP invalidates. The CP code, I wrote should correctly "sync" keep it in sync.
Note, i have added additional tests to confirm
…r.computed.sort used on class with multiple instances.
|
|
||
| // NOTE: This mechanism of dynamically adding/remove DK’s is not public API. | ||
| // I is likely something that should eventually be explored, | ||
| cp._dependentKeys.push.apply(cp._dependentKeys, sortDependentKeys); |
There was a problem hiding this comment.
I don't understand this. Calling .property('some', 'dep', 'keys') on a CP updates its dependent keys, this is definitely public API...
There was a problem hiding this comment.
@rwjblue well .property replaces, it doesn't merge/append.
stefanpenner
force-pushed
the
fix-for-pr-12215
branch
2 times, most recently
from
8782e45
to
71102c3
Compare
|
There is still an issue with recursion. We can solve it two ways.
I prefer 2 and will likely explore that after lunch. |
| obj2.notifyPropertyChange('sortedItems'); // invalidate to flush, to get DK refreshed | ||
| obj2.get('sortedItems'); // flush to get updated DK | ||
|
|
||
| obj2.set('items.firstObject.count', 9999) |
stefanpenner
force-pushed
the
fix-for-pr-12215
branch
from
71102c3
to
44e309a
Compare
NOTE: This mechanism of dynamically adding/remove DK’s is not public API.
stefanpenner
force-pushed
the
fix-for-pr-12215
branch
from
44e309a
to
7b5339b
Compare
|
@krisselden has provided what i believe to be a failing test scenario for what i was unable to get myself: http://jsfiddle.net/mbd04xee/ I'll try and work the solution in the next few days. I have a pretty good idea how to solve it. |
Resolves a regression in `Ember.computed.sort` that has existed since 2.0.0. The regression occurred when there were multiple instances of a class using `Ember.computed.sort` and caused the sorted value to stop updating. Closes emberjs#12212. Closes emberjs#12215. Closes emberjs#12221. Closes emberjs#12516.
Resolves a regression in `Ember.computed.sort` that has existed since 2.0.0. The regression occurred when there were multiple instances of a class using `Ember.computed.sort` and caused the sorted value to stop updating. Closes #12212. Closes #12215. Closes #12221. Closes #12516. (cherry picked from commit f05bd2d)
