Hyperapp: Transitions broken in 1.2.6

[zaceno]

Between 1.2.5 and 1.2.6, this snippet was introduced in the child-patching algorithm:

        if (newKey != null && newKey === getKey(oldChildren[i + 1])) {
          if (oldKey == null) {
            removeElement(element, oldElements[i], oldChildren[i])
          }
          i++
          continue
        }

In cases where all children are keyed, and the first one is removed, I believe this somehow causes onupdate not to be called for the rest of the children. (Not exactly sure how yet)

Why do I think this?

The behavior can be observed in the "Toasts" example of @hyperapp/transitions: https://codepen.io/zaceno/pen/QOYOZd

Add a few messages (like five) Click the messages away from the top.

Notice that the remaining children don't slide up smoothly as they should. Rather they skip up immediately. This means that the Move transition isn't being triggered.

Also, notice that the fade-out animation for removed toasts doesn't occur from their current position at the top, but from their original position (before you started removing them). This tells me that the position-tracking isn't being triggered either.

Both the Move transition and position tracking are triggered by the onupdate lifecycle method. So my conclusion is that onupdate is not being called.

Cross-reference: https://github.com/hyperapp/transitions/issues/19

[zaceno]

@SkaterDad If I'm not mistaken, this was an optimization from you. Does the notion that it might cause onupdate not to be called in the other children (that aren't removed) give you any ideas?

[zaceno]

One theory could be that perhaps removeElement in the snippet above expects the element to be removed immediately?

-- That is not always the case. When using onremove lifecycle events, the event-handler may defer removal for a while. (Like the Exit transition does, in the "toasts" example above)

[SkaterDad]

@zaceno @SkaterDad If I'm not mistaken, this was an optimization from you. Does the notion that it might cause onupdate not to be called in the other children (that aren't removed) give you any ideas?

Yep, that was my contribution. It fixed an issue I was having where <input> elements in a form would lose focus if you toggled an element right before them. As a nice side effect, it vastly improved some benchmarks also by preventing the VDOM from moving more elements than it needed to for simple cases.

I've done some experimenting with animations in hyperapp, but never came up with a solution I was happy with. Trying to do it with hyperapp's VNode lifecycle hooks is very challenging, and leaves your implementation at the mercy of the patch algorithm.

I think in elm, animations are all done via application state, since you can't hook into the VNode lifecycles.

In some other libraries, like snabbdom, there are more ways of hooking into the VDOM to achieve these animations.

In Vue, to have a re-ordering animation you have to use the <transition-group> component, which manages the animations of its children. Could something like that be an option?

[jorgebucaran]

The problem with onremove is one of the things #499 is going to address. I think we should remove onremove until that is sorted out.

[jorgebucaran]

@SkaterDad I think in elm, animations are all done via application state, since you can't hook into the VNode lifecycles.

Do you think you could expand on this (if you have any experience with animation in Elm)?

Do you mean using the state to flip style attributes?

[SkaterDad]

@jorgebucaran Do you think you could expand on this (if you have any experience with animation in Elm)?

I don't have much first-hand experience, but I've read through some example code. Since elm is very locked down, the libraries can't mess with DOM nodes directly, so the animation state is kept in the model, and applied by style attributes.

For my own hyperapp apps, I've just stuck with CSS animations/transitions so far. I'd prefer to do more elaborate stuff in the future, but I tend to go down very deep rabbit holes when I focus on non-feature work :laughing:

[zaceno]

@SkaterDad Not sure if you were aware, but when I said "Transitions broken" in the title of the issue, I was referring to @hyperapp/transitions the library. It's true that using the lifecycle events for css-transitions is pretty tricky and "at the mercy of the patch algorithm". That's basically what @hyperapp/transitions compensates for, by tracking element positions. It works pretty great actually, as long as onupdate is called (which is what stopped happening in this case).

Anyway, maybe you knew all that. Just making sure we're talking about the same thing.

So, back to the issue at hand here: When I remove a keyed node (but the element itself is removed later), it makes onupdate not called on the other children. I wonder: why? and is there an easy fix for it?

[zaceno]

@jorgebucaran onremove is not the immediate problem here (indirectly it may be... don't know). It seems for some reason onupdate is not called on the children in this case. That's why I'm saying this is a bug in Hyperapp, not specifically about transitions.

The fix for the bug might turn out to be to disable the delayed-remove feature of onremove. That would be a shame because it would leave @hyperapp/transitions permanently broken (and it wouldn't fix my problem 😛 ) But that's probably still better than leaving it in a semi-broken state.

[zaceno]

It's off-topic, but I too would be interested in state-based patterns for managing transitions in Hyperapp. I've tried before but not managed to make something reusable & general out of it.

[SkaterDad]

Yep, I figured you were talking about @hyperapp/transitions.

Looking into hyperapp's source again, I would expect this function still gets called for all of the children:
https://github.com/hyperapp/hyperapp/blob/master/src/index.js#L304

The isRecycling flag should be false by then, so it's confusing why onupdate wouldn't get added to the lifecycle queue.

[zaceno]

EDIT: I retract the comment posted here earlier (see the history). My test was wrong. I was forgetting that the app is not done rendering the first time immediately after the app call

[zaceno]

Here's the test I wrote to try to replicate the behavior in the toasts example.

test("onupdate bug?", done => {
    var count = 0
    const onupdate = () => { count++ }
    const onremove = () => {} //noop
    const state = { toggled: true }
    const actions = {toggle: () => ({toggled: false})}
    const view = state => h('ul', {},
        state.toggled
        ? [
            h('li', {key: 'a', onremove}, ['a']),
            h('li', {key: 'b'}, ['b']),
            h('li', {key: 'c'}, ['c']),
        ]
        : [
            h('li', {key: 'b', onupdate}, ['b']),
            h('li', {key: 'c', onupdate}, ['c']),
        ]
    )
    const {toggle} = app(state, actions, view, document.body)
    setTimeout(_ => {
        toggle()
        setTimeout(_ => {
            expect(count).toBe(2)
            done()
        }, 200)
    }, 200)
})

It PASSES -- meaning my analysis of what is happening is probably wrong.

I still think onupdate is not being called for some reason, in that particular case. But I'm not sure why.

[SkaterDad]

@zaceno I find it easier to debug all of this stuff in a Codesandbox, with hyperapp's source right in the project.

Here's one I just set up with your toasts example. https://codesandbox.io/s/km47mp187o

I wrapped the onupdate functions to log out the properties when they're called (look for the "wrapped" function in the hyperapp.js file), and it looks like onupdate gets called for all of the elements on each render.

[zaceno]

Thanks @SkaterDad, and good debugging tip! I actually do that too often... wonder why I didn't think of it this time 😜

So you've confirmed that onupdate is called properly. So I'm out of concrete ideas 😓

Another observation one could make is that it's only observed in the Toasts example. Not the other transitions examples. That's not a huge spread of cases, but one thing that's unique about the Toasts example is that it uses deferred removals, in a set of multiple children . (The caursel example also uses deferred removal, but it's only removing the one child)

There is a bug that I've seen only very rarely, where some elements aren't removed properly. That's not exactly what's occuring here (because all elements are eventually removed when you click them). But perhaps it's a new flavor of that bug, produced by the new code in the patching algorithm.

(Please note: I'm not advocating we remove that code. It should stay. I'm just trying to understand what's happening, and the only (seemingly) relevant diff between 1.2.5 and 1.2.6 is that)

[jorgebucaran]

Here is a start: https://github.com/jorgebucaran/superfine/tree/store-elem-in-vnode

If I can get somewhere I like with that branch and use my time well, I may be able to include a partial diff rewrite in 2.0.

[zaceno]

Current status of this issue: waiting for superfine's new vdom to be ported to hyperapp as it fixes a bug. The bug was present before hyperapp v1.2.6 as well, but likely exacerbated in v1.2.6, and so that's when I started observing it in @hyperapp/transitions.

https://github.com/hyperapp/hyperapp/issues/737

[jorgebucaran]

A current focus is on shipping V2, so I think I'll never be able to get to this, even after V2. 🙏