arrayComputed bug when sorting by fields affecting dependent computed.filterBy

[go1dfish]

http://jsfiddle.net/nx00423k/

Check/uncheck Kris Seldon. Once you click him he should vanish, but he doesn't. Things get really weird on subsequent clicks

This example is simple and silly, but you can imagine a more realistic example with a Ember.Select instead of a boolean field here.

---

The issue appears to be that if you:

1. Have a Ember.computed.filter or Ember.computed.filterBy that observes and acts on a property
2. Have a Ember.computed.sort that is looking at the same property as above

Then you can run into situations where property 2 has items that are no longer in property 1.

[drogus]

It's worth noting that it throws "Uncaught Error: Index out of range". I've seen this error when I was writing my own array computed helper. I thought then that it's a bug on my side, but I didn't have time to debug it. I think I still have a failing test with more isolated (ie. only one arrayComputed function) case somewhere, I'll look for it later today,

[go1dfish]

I've run into all sorts of weirdness that's difficult to track down when stacking arrayComputed properties together. All we can do is try to narrow it down to the simplest examples of things that make us go WTF and work from there.

#5319 and #5268 are worth looking into as well.

[drogus]

I've investigated this further and it seems that the problem lays in the timing of calling removedItem. This issue can be reduced to the following script:

var obj = Em.Object.extend({
    collection: [
      {firstName: 'Kris', lastName: 'Selden', isRouterMan: false},
      {firstName: 'Luke', lastName: 'Melia', isRouterMan: false},
      {firstName: 'Formerly Alex', lastName: 'Matchneer', isRouterMan: true}
    ],
    mortalsSort: ['isRouterMan'],
    unsortedMortals: Em.computed.filterBy('collection', 'isRouterMan', false),
    mortals: Em.computed.sort('unsortedMortals', 'mortalsSort')
}).create();

var kris = obj.get('mortals.firstObject');
var anotherObj = Ember.Object.create({
  kris: kris,
  isRouterManBinding: 'kris.isRouterMan'
});

// this would work: kris.set('isRouterMan', true)

anotherObj.set('isRouterMan', true);

When isRouterMan property is set on the object directly it works correctly, but when it's set through a binding, it will fail. I'm not yet sure what exactly happens there, but in the binding case when the removedItem callback is called, changeMeta.previousValues is null here:

ember.js/packages/ember-runtime/lib/computed/reduce_computed_macros.js

Line 790 in 5ddaa3e

if (changeMeta.previousValues) {

Because of that, object's value of isRouterMan property is used and binary search fails to find the proper item. In this particular case there are 2 items in the filtered array and binary search returns index of 2.

I will try to narrow it down even more and then fix it, any help is appreciated. I'm trying to get a better understanding of ember-metal and ember-runtime.

[drogus]

I've been able to narrow it down and I think I have a fix, although I'm not sure it's correct. It doesn't break any tests, but I'm just starting to understand what's going on in Ember.js internals.

Basically the problem is that dependentArrayWillChange in reduce_computed.js creates a ChangeMeta object without passing previous properties values. It results in a problem with binary search explained in my previous comment. This code fixes it:

diff --git a/packages/ember-runtime/lib/computed/reduce_computed.js b/packages/ember-runtime/lib/computed/reduce_computed.js
index 62a4a1d..13b7ec4 100644
--- a/packages/ember-runtime/lib/computed/reduce_computed.js
+++ b/packages/ember-runtime/lib/computed/reduce_computed.js
@@ -248,7 +248,7 @@ DependentArraysObserver.prototype = {
     if (this.suspended) { return; }

     var removedItem = this.callbacks.removedItem;
-    var changeMeta;
+    var changeMeta, changedItem, previousValues;
     var guid = guidFor(dependentArray);
     var dependentKey = this.dependentKeysByGuid[guid];
     var itemPropertyKeys = this.cp._itemPropertyKeys[dependentKey] || [];
@@ -273,7 +273,10 @@ DependentArraysObserver.prototype = {

       forEach(itemPropertyKeys, removeObservers, this);

-      changeMeta = new ChangeMeta(dependentArray, item, itemIndex, this.instanceMeta.propertyName, this.cp, normalizedRemoveCount);
+      if(changedItem = this.changedItems[guidFor(item)]) {
+        previousValues = changedItem.previousValues;
+      }
+      changeMeta = new ChangeMeta(dependentArray, item, itemIndex, this.instanceMeta.propertyName, this.cp, normalizedRemoveCount, previousValues);
       this.setValue(removedItem.call(
         this.instanceMeta.context, this.getValue(), item, changeMeta, this.instanceMeta.sugarMeta));
     }

I'll work on a proper PR tomorrow (it's getting late here) unless someone sees any problem with a proposed change.

[drogus]

@go1dfish do you have any more examples of problems with stacked arrayComputed properties? My patch fixes the case you provided in original jsfiddle, but I'm really curious if it fixes also any other bugs, or if it requires more changes (in the latter case I'm happy to work on them, too).

[sandstrom]

I've got a possible similar issue, outlined here:
https://gist.github.com/sandstrom/1f3378d3927d6286aa27

It's with computed.filter, which is built on arrayComputed.

[drogus]

@sandstrom I think this is a bit different, seems more like #4423

[sandstrom]

@drogus right, thanks! Interestingly, I've commented on that issue a few months ago but apparently my memory isn't with me today 😄

[go1dfish]

@sandstrom Yeah your problem is actually for the most part expected behavior of computed.sort (well it would be nice if a property that had a change event but didn't change array positions didn't cause removes/adds)

It's just that the expected behavior of computed.sort isn't exactly desirable if you want to direct edit things that you're sorting by. You have to either buffer your edits somehow like in an object controller intermediate property; or control exactly when you refresh your list.

@drogus The only other narrowed down specific cases I can think of are already linked on this thread. I haven't had a chance to try your patch yet but hopefully I can this week.

[wagenet]

@go1dfish get a chance to look at this?