fix(view-slot): add null check to removeAt

[AshleyGrant]

Per @jods4 this can be considered a breaking change.

[StrahilKazlachev]

This will help only in the cases where the return value is not used. Else you will just get something like Can not read property "X" of undefined elsewhere.

[AshleyGrant]

I address this in a comment here: #574

There's a fundamental issue with the async nature of Aurelia's templating/binding system and having one template controller directly inside another. For example,

<template>
  <table if.bind="array.length > 0">
    <tr repeat.for="item of arrray">
       <td>${item}</td>
       <td><button click.delegate="removeItem(item)">Delete Item</button></td>
    </tr>
  </table>
</template>

If the delete item button is clicked on the last item in the array, then the repeater needs to remove that tr element in the same turn of the binding system that the if attribute needs to completely remove the table from the rendered view. It doesn't seem there is currently anything that makes sure the inner template controller's view removal happens before the outer template controlller.

In any case, if this requires a major version bump of the templating library, then so be it. This has been a pain point for developers pretty much since Aurelia existed, as evidenced by the large number of issues reported for it.

[StrahilKazlachev]

If this PR is getting merged, lets fix the return type of .remove, it should be the same as of .removeAt.

[AshleyGrant]

Done.

[StrahilKazlachev]

Another approach, either make .children public or add .hasView/hasViewAt, then the caller can do a check before calling .remove/.removeAt. Won't deny it's more work.

[AshleyGrant]

@EisenbergEffect or @jdanyow do you have any opinions on this?

[EisenbergEffect]

@AshleyGrant Do you have a gist that demonstrates the error? Will the exact code above produce the error?

[AshleyGrant]

I can't seem to get a simple example to produce the error. It ends up needing a couple of if and repeats and composes to make it happen. I have it consistently erroring in my client app and I can show you that if need be. It's a fairly complex setup with dynamically built UI based on a model that describes the UI. The problem surfaces when removing items from an array.

[EisenbergEffect]

I didn't think the sample above would cause the error. I need something where we can see this in isolation.

[AshleyGrant]

Trying to cook that up.

[compgumby]

I have a similar issue. I have a div that repeats for some items. An element of the repeating div is a input[radio]. Its checked.bind="$parent.selected" and the model.bind="$index"".

When a user selects the radio button and clicks the "Delete" button at the bottom of the page, I simply:

this.listOfItems.splice(this.selected, 1)

If I select the first element of the array (first of the repeater), and click "Delete", it calls -> this.listOfItems.splice(0, 1)

KABOOM

The array is correct and intact with the 1 element sliding into the 0 position... but the repeat fails to render. If I leave the view and come back to it, it redraws correctly.

If I select ANY OTHER element and call the splice, I get this stack trace - the element appears to go away properly, but the console shows:

aurelia-templating.js:1544 Uncaught TypeError: Cannot read property 'animatableElement' of undefined
    at getAnimatableElement (aurelia-templating.js:1544)
    at ViewSlot.animateView (aurelia-templating.js:1583)
    at ViewSlot.removeAt (aurelia-templating.js:1785)
    at Repeat.removeView (repeat.js:263)
    at ArrayRepeatStrategy._runSplices (array-repeat-strategy.js:193)
    at ArrayRepeatStrategy.instanceMutated (array-repeat-strategy.js:160)
    at Repeat.handleCollectionMutated (repeat.js:151)
    at Repeat.call (repeat.js:95)
    at ModifyArrayObserver.callSubscribers (aurelia-binding.js:295)
    at ModifyArrayObserver.call (aurelia-binding.js:908)

Just another data point.

[bigopon]

So far, all errors come from Repeat, which I think could probably caused by instanceMutated issue, which was fixed recently. If it's actually the case, then there is no need to change here, as the behavior is quite good - a hard failure when removing with invalid index.