[BUGFIX beta] Ember.computed.sort Observers

[mmpestorich]

This commit ensures that observers are only added once 'sortPropertyDefinitions:change'.

Currently whenever the sortPropertyDefinitions change and the computed property is recomputed duplicate observers are being added for this property. This eventually causes performance issues if the sortPropertyDefinitions change frequently as illustrated here: http://jsbin.com/tutoni/2/edit

[machty]

ping @hjdivad for review

[mmpestorich]

In current master, the observer being added whenever sortPropertyDefinitions property changes (i.e. when it is set to a new array) doesn't replace the previous one but rather registers an additional one. That causes the arrayComputed to recompute by a multiple of the number of observers currently registered on sortPropertyDefinitions (the more you set sortPropertyDefinitions the greater number of times it recomputes each time the array is set to a new one). It should only be recomputing one time but that's definitely not the case. The same thing is happening with the @each observers on sortPropertyDefinitions as well. The bottom line here is that Ember.computed.sort is recomputing itself way to many times. This jsbin that illustrates what I am seeing in master: http://jsbin.com/tutoni/11/edit?js,output

This latest commit I believe addresses the issues with both the property and each observers. Noew, when Ember.computed.sort recomputes it will properly remove the previously registered observers before setting up new ones. This jsbin shows the same scenario as above but with this fix: http://jsbin.com/pavoy/5/edit?js,output

I am not sure how to write a test to ensure the Ember.computed.sort only recomputes once when its sortDefintionProperties property or contents change. recomputeOnce is hidden by the ReduceComputedProperty constructor and that would be the function that we need to ensure only gets called once...

[stefanpenner]

please add tests

[wagenet]

@mmpestorich ping.

[mmpestorich]

@wagenet @stefanpenner Added tests that fail in master with the behavior illustrated in the jsbins above; the tests pass with this commit.

[stefanpenner]

@mmpestorich awesome thank you. I am unfortunately traveling, @hjdivad could use your eyes on this one!

[wagenet]

See also #9462

[stefanpenner]

Thank you for your contribution, unfortunately this PR is no longer applicable. As ember has deprecated (and removed support for) array computed/ reduce computed.

Luckily, going forward glimmer handles efficient diffing for us, so the faster and naive resort/refilter the collection approach is sufficient. As a bonus it is also less complicated and without the same bugs.