fix(array-repeat): olditem bindingcontext not available

[zewa666]

fix an issue where due to delayed rendering the bindingContext of the oldItem might not be available

fixes issue aurelia/animator-velocity#21

[EisenbergEffect]

@martingust @jdanyow Could one of you review this? I'm afraid you are the only two who know how this part works :)

[Alexey1]

I think it is worth mentioning that when I apply this fix in my dev, I have the setup described in aurelia/animator-velocity#21 working as expected but when I modify my code as shown below I get another issue related to animations and repeater in a similar way.
In summary the component class field bound to "element.ref" attribute in template is "null" in "attached" method while it should reference corresponding DOM element.

Except code changes the setup is the same as in aurelia/animator-velocity#21 .

main.ts (same as aurelia/animator-velocity#21 )

import { Aurelia } from 'aurelia-framework'
import environment from './environment';

export function configure(aurelia: Aurelia) {
  aurelia.use
    .standardConfiguration()
    .plugin('aurelia-animator-velocity', instance => {
      instance.enterAnimation.options.duration = 1000;
      instance.leaveAnimation.options.duration = 1000;
    })
    .feature('resources');

  if (environment.debug) {
    aurelia.use.developmentLogging();
  }

  if (environment.testing) {
    aurelia.use.plugin('aurelia-testing');
  }

  aurelia.start().then(() => aurelia.setRoot());
}

app.ts

export class App {
  private toggle = true;
}

app.html

<template>
  <require from="./item-details"></require>
  <label>
  <input type="checkbox"
    checked.bind="toggle">
    toggle
  </label>
  <div>
    <div class="au-animate"
      if.bind="toggle">
      <div repeat.for="i of 3"
        class="au-animate">
        <item-details index.bind="i"></item-details>
      </div>
    </div>
  </div>
</template>

add item-details.html file in /src/ folder

<template>
  <div element.ref="detailsEl">${index}</div>
</template>

add item-details.ts file in /src/ folder

import { bindable } from "aurelia-framework";

export class ItemDetails {

  @bindable index: number;
  detailsEl: Element;

  attached() {
    console.log('attached', this.index, this.detailsEl);
  }

  detached() {
    console.log('detached', this.index);
  }
}

au run, load page, see "toggle" checkbox, list of three items. Click "toggle" checkbox twice under one second, see output in console:

attached 0 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​0​</div>​
attached 1 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​1​</div>​
attached 2 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​2​</div>​
detached 0
detached 1
detached 2
attached 0 null
attached 1 null
attached 2 null

expected output

attached 0 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​0​</div>​
attached 1 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​1​</div>​
attached 2 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​2​</div>​
detached 0
detached 1
detached 2
attached 0 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​0​</div>​
attached 1 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​1​</div>​
attached 2 <div element.ref=​"detailsEl" class=​"au-target" au-target-id=​"1">​2​</div>​

[zewa666]

hmm seems that return null if not available was not the right way to go. Guess an empty bindingContext needs to be returned @jdanyow @martingust ?

[Alexey1]

After some investigation I found that issue in my comment above doesn't need this pull request to reproduce because the example causes the use of NumberRepeatStrategy instead of ArrayRepeatStrategy and this pull request is updating ArrayRepeatStrategy only. I used array for the repeater locally so I indeed needed to apply change from this pull request to see issue very similar to one in my comment above but when I was posting I changed array to number to simplify code a bit which caused change of what RepeatStrategy is used which I didn't realize. I plan to create I have created a separate issue thread #298 with example above using number for repeater since for that case this pull request is not needed to reproduce the issue.

[zewa666]

Good catch. I'll take a look at the numberrepeater with your example tomorrow. So it sounds like the actual fix in this PR regarding the array repeater itself is fine?

[Alexey1]

All I can say is that the issue described in my first comment here which also is copied here #298 doesn't cast any shadow on possible correctness of this PR.

But to be clear #298 reproduces in both cases:

1. NumberRepeatStrategy is used.
2. This PR is applied and ArrayRepeatStrategy is used.

It's just without this PR issue aurelia/animator-velocity#21 happens first for ArrayRepeatStrategy and so #298 looses its chance to reproduce but potential is there.

With what I've seen so far I believe the problem is not in NumberRepeatStrategy neither in ArrayRepeatStrategy. It is that when Repeater is unbound, it launches children leaving animations and it gets problematic state when it is bound again before children leaving animations finish. Maybe it's reasonable to skip children animations when Repeater is unbound. I applied this idea in my dev with a very small change and it fixes both #298 and aurelia/animator-velocity#21 . Given my limited knowledge of the framework I don't claim to have high degree of confidence if the change is stable overall. I plan to give it another thought and if I don't find nothing new then I will propose a PR with it and let someone with enough knowledge to decide if it is good or not when I figure out how to create PRs properly.

[Alexey1]

I found issue with tests for repeat #300 and I don't see a straightforward way to make appropriate changes in the tests until it is resolved so instead of creating PR I'll just share here the change that fixes both #298 and aurelia/animator-velocity#21:

repeat.js file

  unbind() {
    this.scope = null;
    this.items = null;
    this.matcherBinding = null;
    //replace this line
    //this.viewSlot.removeAll(true);
    //with this one
    this.viewSlot.removeAll(true, true);
    this._unsubscribeCollection();
  }

This causes repeater to skip leaving animations for children. Otherwise the repeater gets problematic state if it is bound again until leaving animations are complete. In the case I had, repeater DOM elements are already removed at that point so it seems reasonable to skip animations. But I can't be sure if there is no other case where it might be required, so input is needed from someone with better knowledge of the framework.
If my guess is right and it is accepted, then the current PR might not be needed.

[EisenbergEffect]

@zewa666 Where are we at on this?

[zewa666]

@EisenbergEffect this one's not necessary anymore.

[EisenbergEffect]

Thanks!