'Index out of range' error using Ember.computed.sort with multiple sort properties
#9278
Comments
|
@slindberg Thanks for this. Yes, it is a nasty one. Our previous commit 6c4079f should probably have never made it into beta. @mattraydub and I have been aware of this for a little while now and temporarily worked around it by doing exactly what you proposed in 4 above; eliminating the addedItem: function (array, item, changeMeta, instanceMeta) {
var index = instanceMeta.binarySearch(array, item);
//array.insertAt(index, item);
if (array.indexOf(item) === -1) array.insertAt(index, item);
return array;
},
removedItem: function (array, item, changeMeta, instanceMeta) {
//var proxyProperties, index, searchItem;
//
//if (changeMeta.previousValues) {
// proxyProperties = merge({ content: item }, changeMeta.previousValues);
//
// searchItem = SearchProxy.create(proxyProperties);
//} else {
// searchItem = item;
//}
//
//index = instanceMeta.binarySearch(array, searchItem);
//array.removeAt(index);
array.removeObject(item);
return array;
}However, I don't belive that that eliminating
I believe, in order to move past the second issue, it's going to be necessary to create a cache of sort property values so that a proper comparision between a |
|
Thanks @mmpestorich, glad to hear you're on top of it 😄 |
|
does array/reduce computed use Ember.Map or Ember.OrderedSet internally? cc @hjdivad |
|
@stefanpenner Not that I can see. Internally, Originally, I thought that exposing |
|
This bug was really bugging me. Please see the above PR and let me know what you think. |
|
Was this fix going to be in 1.8.0? I just updated to 1.8.1 and still get the "Index out of range" error on arrays that are using the computed.sort with any number of sort properties |
|
@electricgraffitti it looks like the bugfix only made it in at v1.9.0-beta.1 |
|
Sorry to comment on a closed issue, but I just updated to 1.9.1 hoping to catch this bug fix. Unfortunately my app is still getting the |
|
@kelsin yes, there is a easy way to see: click on the PR (#9356), then find where it got merged and click on the merge commit (7b270f8). At the top , below the commit description, you can see that the commit is in |
|
Ok so my bug must be a variation of this one. Thanks so much! |
|
I also have a bug that I think is very closely related to this one - here is the stack trace: I'm not sure where to even start debugging this - should I start a new issue for it? |
|
@kimroen if you're using Ember 1.9 or newer, please file a new issue. Otherwise try upgrading first. |
This is a nasty one. The issue occurs inside the
removeItemarray computed callback when finding the correct index for an item whose observed sort property changes. The binary search can return a bad index due to the internalorderfunction not properly reporting the item's order. This can happen when 1) there are multiple sort properties, 2) only one of them changes, 3) the previous values of the changed sort property were equivalent, and 4) the change moves the item to the other side of the midpoint of the array. This is due to a change introduced in this commit, where the sort property is looked up on the item withoutEmber.getwhen it is aSearchProxy.The problem is that the
SearchProxyinstance is created with thechangedPropertiesmeta object, which obviously does not contain unchanged sort properties. This means that when the unchanged property is looked up directly in this line:It uses
undefinedas the comparison instead of that actual sort property.jsbin
The issue that the bugfix commit corrects is still valid, so I see a few ways to address this new one:
hasOwnPropertycheck for the sort property on the proxy object before accessing it directly. More overhead, but simple.removeItem. Probably more overhead than the previous solution, but still simple.SearchProxysomehow. Not sure if this is possible since it would likely require overridingget.removeItemand just usearray.indexOf(...). I assume this was done originally for performance on large lists, so this is probably out.@mmpestorich, @hjdivad: thoughts?
Also possibly related to: #4831
The text was updated successfully, but these errors were encountered: