-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ember.computed.sort has serious flaw in 2.x+ #12212
Comments
One possible solution would be to filter out foreign observers before calling if (cp._sortPropObservers) {
let thisObservers = cp._sortPropObservers.filterBy('firstObject', this);
thisObservers.forEach(args => removeObserver.apply(null, args));
cp._sortPropObservers.removeObjects(thisObservers);
} else {
cp._sortPropObservers = [];
} That said, the current implementation feels a tad bit dangerous memory wise that a class ends up with [potentially dangling] references to instances. Looking for some advice on the best way to fix this. It's blocking us from moving to 2.x at the moment. |
Ah crap ya, this is bad (and my fault). I believe storing the observers on the CP was an omission, it really is a more complex relationship @workmanw if you can provide a failing test i can likely work on it this afternoon or tomorrow morning. |
@stefanpenner Yea, that shouldn't be a problem. I'll do it now. |
thanks |
@stefanpenner Added: #12215 To demonstrate the validity of this test, if you comment out Thanks a lot! |
…r.computed.sort used on class with multiple instances.
@stefanpenner Any progress on this? I know you've got the WeakMap RFC and PR ... unfortunately we cannot move to 2.x until this issue is resolved. |
Been on vacation. Will take a look this Sunday (feel free to pester me on Sunday :p) |
Hehe. Thank you. I know you have a million things on your plate. I'm happy to help if there is any work you want to delegate. But it seems the WeakMap may require low level Ember/Javascript knowledge that I don't possess. |
That pr just needs feature flagging for the public API. If you have cycles to spare, that would push it over the edge |
Demonstrates issues with Ember.computed.sort used on class with multiple instances.
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.
Demonstrates issues with Ember.computed.sort used on class with multiple instances. (cherry picked from commit 8adb04c)
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)
After spending the entire morning I've finally figure this out. It's kind of hard to explain, but let's start with a demo: http://emberjs.jsbin.com/pagoguyofa/edit?html,js,output . Notice that the
{{my-list}}
component usesEmber.computed.sort
and there is two instances of the component in the DOM. Clicking the button to move an item only causes one of the{{my-list}}
component instances to update.To understand why, you have to look at the implementation of
Ember.computed.sort
. First, and most important, the computed macro creates a new computed property for each declaration, not for each component instance. See: reduce_computed_macros.js#L566. This means that every component instance of the same type will ultimately share the same computed property.Next, every time the value is computed, it clears out all prior observers and rebuilds them. See: reduce_computed_macros.js#L568. There inlies the problem because since the computed property lives on the class, it clears all other observers for other instances of the same component.
Does that make sense to anyone?
The text was updated successfully, but these errors were encountered: