Mithril.js: Unnecessary DOM node remove/append

[hankeypancake]

When rendering a list of items from any kind of fixed length array where you append new values and remove the overflowing values, mithril will behave correctly up until the array reaches maxlength, then start removing all DOM nodes and appending them again if they should still be rendered. This will kill all css transitions for styles that changes from state a to state b.

The nodes onremove-hooks will not be triggered

Expected Behavior

Only the removed nodes should be removed from the DOM

Current Behavior

All nodes are removed, and the ones that shouldn't have been removed gets appended again

Possible Solution

I have no idea

Steps to Reproduce (for bugs)

This jsfiddle demonstrates the behaviour: https://jsfiddle.net/hankeypancake/aa4c9gnr/
It has contains two implementations: one with react which handles this correctly, and one with mithril. Clicking one box will restart that particular app.

The issue appears when the boxes are 'full'

If you, before the issue appears, set a DOM node removal breakpoint on one of the later nodes in chrome devtools, you can see that it get's removed

Context

I was just throwing a simple moving time series visualisation together when I encountered it.

Your Environment

• Version used: 1.1.5
• Browser Name and version: Any
• Operating System and version (desktop or mobile): Any
• Link to your project: Private, jsfiddle is provided

[isiahmeadows]

It's actually a key bug - Mithril should know better than to replace keyed subtrees Edit: components that just happen to be moving backwards (i.e. earlier in the DOM).

[isiahmeadows]

After toying around a little, it works with fragments as well as elements. Edit: nope. Ignore that.

[isiahmeadows]

It appears to fail on everything, so I suspect it's an issue with the diffing heuristics and key resolution.

[hankeypancake]

as per the gitter discussion: it works if we reverse the array generation to add from the end instead of add from the start

[pygy]

As I thought the problem lies with the way DOM nodes are handled (that's what I mean when I asked about elements vs components on gitter). The hooks fire normally.

The culprit is the unconditional insertNode line in the mapped diff loop. If you comment it out, the sample works properly (but doing that causes regressions in the test suite IIRC).

I suppose that adding from the end uses another branch...

I think I've already pinged @lhorie about that line in another issue, it weirded me out...

Edit: actually, commenting out that line doesn't break any test, but I expect it to break recycling, we just don't have a test that exercises mapped keyed diff when reviving a pooled node.

Prepending that line with if (shouldRecycle && v.tag === movable.tag) wouldn't hurt. I wonder if the other insertNode calls shouldn't be predicated on shouldRecycle rather than recycling...

Edit again: it does break a test actually (I was editing a folder and testing from another one duh):

    o("moves from start to end", function() {
        var vnodes = [{tag: "a", key: 1}, {tag: "b", key: 2}, {tag: "i", key: 3}, {tag: "s", key: 4}]
        var updated = [{tag: "b", key: 2}, {tag: "i", key: 3}, {tag: "s", key: 4}, {tag: "a", key: 1}]
    // ...

The tags are not re-ordered

:-/

[thysultan]

Here is a small unit spec that exercises this case.

7, 6, 5, 4, 3, 2 [next]
6, 5, 4, 3, 2, 1 [prev]

The least no. of DOM ops would be to create and insert 7 before 6, then remove 1, though based on the fiddles behavior it currently might create(append) 7, move(append) 6, 5, 4, 3, 2 then remove 1. to get to 7, 6, 5, 4, 3, 2.

[hankeypancake]

Could this be added to the 1.2.0 milestone?

[pygy]

@hankeypancake there are other updateNodes bugs that I must fix before tackling this one... Hopefully we'll have i in the next release too.

[pygy]

This is actually a dupe of #1791

Closing this one (it is still on my radar, @hankeypancake feel free to subscribe to the other bug for updates).

[pygy]

So the fix has been merged into next.