Mithril.js: Hook mystery with keyed nodes

[pygy]

@gyandeeps reported surprising diff behaviour yesterday in gitter...

Here's an updated jsbin with a reduced test case: http://jsbin.com/fiziricege/edit?js,console,output

For the issue to occur there must be keys on the parents, hooks on the child, and no hooks on the parent.

Enabling the hooks on left makes them run on left-1 too.

[isiahmeadows]

Here's a much smaller repro.

It appears all you need is to change the key on the parent, and the oncreate gets erroneously ignored on the child. Node type doesn't matter, and that may even change between renders. Identity is irrelevant here, too, in all stages.

[lhorie]

Yeah, I'm already looking into it.

[pygy]

That was fast :-)

[lhorie]

fixed in v1.0.1

Note: you still shouldn't reuse vnodes as per http://mithril.js.org/vnodes.html#avoid-anti-patterns

[lhorie]

@pygy I had the fix mostly ready last night, but had to attend to my crying kid :)

[pygy]

@lhorie I know the drill ;-)

But it looks like there's more: https://codepen.io/anon/pen/LxgebV?editors=0011

Using different elements, toggle twice or more, and oninit doesn't fire on the child anymore. Uncomment oncreate on the parent and it works.

Also, the class names keep on piling up on the child...

Edit, I was actually using a wrong version, but it remains the same with the [current tip(https://cdn.rawgit.com/lhorie/mithril.js/8feb6d3b399277c70bf10dc653abfe3a8d4cb2a8/mithril.js).

[gyandeeps]

Thanks all for help.
Here is the find test: https://jsfiddle.net/a98q7827/3/

This exactly explains what i meant to say, for first run it runs oninit and oncreate for the first time after that it should going to cycle through onupdate and onremove only. Tho DOM nodes are getting removed.

[lhorie]

Ok this is fixed for unkeyed children as well. If you need that specific fix now, I can publish a 1.0.2. Otherwise I'm going to pile a few more fixes before a new release.

[pygy]

@lhorie The class names still pile up...

https://codepen.io/anon/pen/LxgebV?editors=0011 The current class name is prepended to the previous ones

[gyandeeps]

@lhorie were u talking about my use case above? (sorry i think thr are 2 convo gng on in here :smile: )

[lhorie]

@pygy oh sorry I missed your note about class names, let me take a look

[gyandeeps]

@lhorie Thanks for the fix. We use mithril in production and now we are migrating to 1.0 ... But we can wait if you want to pile up fixes and then do a release. Any rough timeline?

[lhorie]

@pygy turns out it's because you're memoizing the attrs. That use case isn't supported. If you move the parentHooks and childHooks into the toggle function it works

[pygy]

@lhorie oooh, my bad, sorry for wasting your time with this... I wouldn't do that in real code, it was a remnant of the Object.assign({key:...}, ...) thing in the JSBin of the first post.

[gyandeeps]

@lhorie Your latest commit does the fix the issue but now i see another issue, here is the test plan

    o("update lifecycle methods work of recycled keyed", function() {
        var createA = o.spy()
        var updateA = o.spy()
        var removeA = o.spy()
        var createB = o.spy()
        var updateB = o.spy()
        var removeB = o.spy()

        var a = function() {
            return {tag: "div", key: 1, children: [ // remove key from here to make this test pass
                {tag: "div", attrs: {oncreate: createA, onupdate: updateA, onremove: removeA}},
            ]}
        }
        var b = function() {
            return {tag: "div", key: 2, children: [
                {tag: "div", attrs: {oncreate: createB, onupdate: updateB, onremove: removeB}},
            ]}
        }
        render(root, a())
        render(root, a())
        o(createA.callCount).equals(1)
        o(updateA.callCount).equals(1)
        o(removeA.callCount).equals(0)

        render(root, b())
        o(createA.callCount).equals(1)
        o(updateA.callCount).equals(1)
        o(removeA.callCount).equals(1)

        render(root, a())
        render(root, a())

        o(createA.callCount).equals(2)  // Reality: 3
        o(updateA.callCount).equals(2) // Reality: 1
        o(removeA.callCount).equals(1)
    })

I hope my expectation is correct.

[lhorie]

@gyandeeps Ok I think I know what's going on. I pushed a fix for update hooks