onremove and key change

[cavemansspa]

Should onremove be invoked in this example: https://jsfiddle.net/6dysnpkL/

let count = 0
m.mount(document.body, {

  view: (vnode) => m('div', {
    key: count, // key changes, view onupdate result in console output
    onremove: (vnode) => {
      console.log('onremove')  // not invoked
    },
    onupdate: (vnode) => {
      console.log('onupdate', vnode)
    },

  }, 'hello ' + count)
})

let id = setInterval(() => {
  count++
  console.log('counter', count)
  if(count > 9) clearInterval(id)
  m.redraw()
}, 1000)

[pygy]

It's a common gotcha, and it took me a while to detect it as well...

Keys only work in list context. Wrap the m('div',...) call in an array and onremove fires.

The tricky thing is that the engine implicitly creates lists in some places but not always.

[barneycarroll]

The route docs advocate using key on single components: http://mithril.js.org/route.html#key-parameter

[pygy]

@barneycarroll m.render(element, vnode) normalizes vnode to [vnode]. So keys work for the root element (and that extends to whatever RouteResolver.render returns). That's what I meant by "the engine implicitly creates lists in some places", and why I labeled it as a gotcha, the docs must be improved...

[barneycarroll]

@pygy the expected behaviour is intuitive, and describing the reasons for the edge case is awkward and unrewarding. How about we change components to always wrap returns in fragments? This would fix the issue transparently and make vnode structure more predictable.

[barneycarroll]

@cavemansspa in the meantime, it's easy enough to ensure all element & component nodes are in lists by wrapping view return values in arrays: https://jsfiddle.net/barney/Lsr7be0y/

[valtron]

Here's another instance of this: https://jsfiddle.net/u9rgLyk2/3/

My intention for using key was to indicate that the new vnode is different, i.e. if keys are different, don't recycle; instead, unmount the old vnode and create a new one. This fixes it nicely.

Another solution would be a new lifecycle method onmatch(vnode, old) (or whatever) called before shouldNotUpdate which, if false is returned, does removeNode(old); createNode(parent, ...). In this case, I wouldn't use key: rather I'd compare the relevant vnode attrs (i.e. options in the chart example) inside onmatch.

[cavemansspa]

@valtron -- did you see the gitter followups https://gitter.im/mithriljs/mithril.js?at=5c3125053cf6c35a8497fd6d and https://gitter.im/mithriljs/mithril.js?at=5c31280e1491b27876c818ff?

[valtron]

@cavemansspa I did, that's just a workaround though, required by the fact that mixing keyed/unkeyed nodes isn't supported/defined. Would be better, IMO, to define and support this behaviour, since the change is so simple.