Adding and then sorting an array breaks repeat binding

[simonfox]

When adding (pushing) to an array and then sorting the result, a repeat binding doesn't update correctly.

I have created a repro here. I have added a repeat binding over a collection to the Welcome view model. There are a couple buttons below, one to add then sort and one to add without sort. If you use add with sort, you will see the unexpected behavior.

[devmondo]

i am facing the same issue

[BruceL33t]

Ye, this is a huge problem for me as well :/ I can't to this.somearray[0] = something, since it is not reflected in the views. On the other hand, pushing works just fine. this.somearray.push(something) works... I hope this will be fixed quickly

[jdanyow]

@BruceL33t your issue is different than what @simonfox reported. Aurelia cannot observe indexed assignment of an array- at least not without Object.observe, which is no longer being championed in TC39. As long as you use methods on the array instance to mutate the array it will work as you expect. In your case, change this.somearray[index] = something to this.somearray.splice(index, 1, something).

[martingust]

This is what I'm experiencing when doing push and sort on the underlying array:

The repeat gets two change records. One for adding 1 view (with index inside the bounds of the array). Another for removing 1 view, on max index +2.

view-slot before making any changes

Second splice index off by +1?

@jdanyow / @EisenbergEffect Any ideas? Potentially looks like the splices are calculated wrong somewhere in calcSplices.

[jdanyow]

I think @simonfox found a case where this is needed.

[martingust]

@jdanyow Concatenating changeRecords and records did fix the error. But the sorting in the DOM is off.

On this line I added this:

if (changeRecords) {
  records = changeRecords.concat(records);
}

Here is a failing integration test.

Let me know if you have any ideas. I can continue on this tomorrow.

[EisenbergEffect]

@jdanyow @martingust I wanted to push out a release today or tomorrow since we've built up a number of fixes. Any idea if this could be fixed by tomorrow? I just want to know if I should wait or not.

[martingust]

@EisenbergEffect I will continue look into this tomorrow, but I have no idea right now what the issue would be so I can't say if it will be fixed or not by then.

[EisenbergEffect]

No problem. The master is safe to release as is though, correct?

[jdanyow]

yep- all tests passing (other than this one)

[EisenbergEffect]

Ok, great. I'll get a release out and then we can get this one in as soon as it's done in a later patch.

[martingust]

@EisenbergEffect @jdanyow I continued trying to trace this down. This is what I have found so far.

The bug happens when doing push and (certain) sort on arrays when the repeat's views require lifecycle. As in earlier comment I tried to concat records with existing records here that fixes the error but the existing records gets duplicated in the viewSlot.

Here is without concatenating records:

 // initial array
 var foo = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];

 // doing push and sort
 foo.push('10');
 foo.sort((a, b) => {});

console.log(foo); // ["0", "10", "2", "3", "4", "1", "6", "7", "8", "9", "5"]

(issue does not happen when the array is shorter)

The repeat gets 3 splices, where each splice have 1 add and 1 delete. First the views get deleted. The first two splices correctly deletes at index 1 ('1') and index 5 ('5') third wants to delete at index 10 (outside array), and error is thrown.

If doing an ugly hack, to skip deleting if index is outside of array, the repeat processes and adds all views to be added, correctly.

In other words; the problem (as I see it) is with the third splice, which should not contain any deletes, just 1 add. I have looked through array-change-records.js without any luck figuring out if something goes wrong here or not. Wild guess is that something is going wrong in there, when calculating the edit distances, perhaps something to do with this comment. I'm not very familiar with this code and not making much progress from here, would need you guys to take a look.

[jdanyow]

@martingust I merged in the integration test you put together and added logic to the array observation to force a flush of the change records prior to doing an array operation that requires a "reset" (sort or reverse). Downside is this will cause DOM changes outside of the task queue but I couldn't think of a way to get around it.

[martingust]

@jdanyow Awesome. I think we can live with that, repeat performance is pretty good in any case.

[simonfox]

@EisenbergEffect any chance of getting a patch release for this fix this week?

[EisenbergEffect]

Yes, I'm waiting to hear about a couple of other issues.

[simonfox]

Awesome thanks @EisenbergEffect

[ben-girardet]

I'm coming across a similar issue and can't get it to work. In an array (this.steps): I need to move an item from a position to another (index previousPosition to newPosition). This array is used in a repeater in the view.

Here are the relevant parts of the code I've tried:

steps.js (VM)

// Option 1 (using splice)
this.steps.splice(newPosition, 0, this.steps.splice(previousPosition, 1)[0]);

// Option 2 (using sort)
let index = 0;
for(let step of this.steps) {
  step.position = (index < newPosition) ? index : index+1;
  if(index == previousPosition) {
    step.position = newPosition;
  }
  index++;
}
this.steps.sort((a,b)=>{
  return a.position - b.position
});

steps.html

<li repeat.for="step of steps">...</li>

Both options have problems when the view gets updated. Sometime the moved item gets duplicated, sometimes the array gets an empty new item, sometimes an item gets missing, ...

Is this stil a bug in the binding system or am I doing something wrong when updating the array ?

[EisenbergEffect]

@jdanyow @martingust Can you guys look into this?

[Tarpsvo]

Experiencing the exact issue @martingust described in detail here earlier, using the latest version.

[insidewhy]

I'm seeing this also. More generally I'm seeing Aurelian fail to maintain the array order with the order on screen if I do many splices and pushes in a row. :(

[insidewhy]

Oh and latest version here too.

[insidewhy]

My perspective on the importance of this bug is that Aurelia has never been able to maintain order of my lists in the view due to various bugs, two of which have been fixed but this one remains.

My website is all about sorting things by score. In the year I've been using Aurelia it's never been fit for this simplest of purposes. That coupled with dealing with so many bugs and a lack of documentation has left me feeling extremely regretful about my choice.