Mithril.js: m.route.link should work with dynamic onclick-handlers

[StephanHoyer]

works

function onclick() {
  alert('huhu')
}

m('a', {
  href: '/foo',
  onclick: onclick,
  oncreate: m.route.link
})

does not work

m('a', {
  href: '/foo',
  onclick: function() {
    alert('huhu')
  },
  oncreate: m.route.link
})

[StephanHoyer]

also works with

m('a', {
  href: '/foo',
  onclick: function() {
    alert('huhu')
  },
  oncreate: m.route.link,
  onupdate: m.route.link
})

I assume the click-handler differs on second redraw and the clickhandler is reboud. Therefor the monkey-patched clickhandler from m.route.link gets overwritten again. Doing this again onupdate solves this by reapplying the link-patch

[pygy]

@StephanHoyer Do you think we can do other things beside #1712?

I don't think we could improve m.route.link to work for dynamic href attributes without onupdate.

[isiahmeadows]

@pygy Simply fixing this to use addEventListener instead (something not easily removable) would fix it.

[tivac]

It's weird that it doesn't already since render uses addEventListener.

[pygy]

I don't think the engine strips the onclick handler, because it is registered on the DOM element directly, the render engine is not aware of it.

The event handler defined by m.route.link(thisVeryVnode) cannot AFAICT be universally made not to depend on thisVeryVnode.

Currently, m.route.link mutates the href of vnode.dom to a prefixed verison of thisVeryVnode.attrs.href and the handler reads back from the DOM attribute. This works fine as long as the vnode.attrs.href doesn't change, but when it does, the prefixed version is replaced by a non-prefixed one. So it probably works with only oncreate for dynamic hrefs when the prefix is "".

What we should probably do is verify that when the handler fires, vnode.DOM.href.slice(prefix.length) === thisVeryVnode.attrs.href, and error out if it isn't the case.

[isiahmeadows]

@StephanHoyer Try to repro it without using m.route.link. Something like this should be adequate:

function oncreate(vnode) {
    vnode.dom.onclick = function () {}
}

// In the rendering component
m('a', {
  href: '/foo',
  onclick: function() {
    alert('huhu')
  },
  oncreate: oncreate
})

I may have been wrong in my initial guess, now that I think about it. This sounds like a rendering bug either way.

@tivac I equally find the use of vnode.dom.setAttribute("href", ...) over vnode.dom.href = ... odd, but that's independent of this issue.

[barneycarroll]

I think the problem with m.rouite.link as a lifecycle method is that the scope of a routed link is greater than any one lifecycle. If you want to extend it to meet the edge cases, you kinda need onupdate - you also need to mutate href after the fact and second guess what the author may be doing with onclick, which all feels very hacky because it's really out of keeping with the principles of the virtual DOM abstraction & hyperscript.

A more extensible solution, that would make it easy to consider things like #1731, would be to provide a route link component. This would avoid the ugly DOM thrashing in the present implementation. The m.route.link documentation could then point to m(m.a, attrs, text) for the cases where routed links may not have static hrefs, or require extra onclick or oncreate behaviour.

[isiahmeadows]

@barneycarroll I almost suggested it should've been a component, but I wasn't sure how people would react here, considering how boilerplatey many component-based routing systems are. (React Router is notably terrible for this. It takes ~30 lines of code for just setting up basic routing.)

[barneycarroll]

@isiahmeadows the reason I'm suggesting this is so we can reduce code in author land and core.

[isiahmeadows]

@barneycarroll I get that. I just had reservations about how well it'd be received, due to bad experiences with other component-based routing systems (like React's), so I was a bit jaded there. And the potential boilerplate I was referring to was on the user's side, not necessarily the implementor's side.

[isiahmeadows]

I'm not sure about m.route.a (the name of your component) though...

[barneycarroll]

I'm not sure about m.route.a (the name of your component) though...

Me neither. Truth be told, I'd like this to replace the m.route.link reference and constitute a breaking change. Having both APIs is silly when one is simply the caveat-free version of the other using a completely different mechanism.

The other possibility is to extend the m.route.link function with the view property that allows it to be both. I think it's fair to say this is totally out of keeping with v1's API style, given how m.route has ditched all the overloads for separate properties.

[isiahmeadows]

@barneycarroll Here's how you could solve that (overloading m.route.link to be a component):

1. Make m.route.link a factory component.
2. Detect factory usage via vnode.tag === m.route.link
3. ???
4. Profit?

And although yes, overloading is generally avoided in Mithril's APIs, if we plan to deprecate current m.route.link usage for v2, this may be okay.

[pygy]

@isiahmeadows that's brilliant :-) :+1:

[barneycarroll]

@isiahmeadows the factory aspect is orthogonal, if anything it's easier to distinguish m.route.link used as a function (lifecycle invocation) from usage as a tag (view property) and avoid any kind of magic sniffing. No?

[pygy]

@barneycarroll The current component kind detection code will treat a function without a view on the prototype as a closure component.

[pygy]

So,

m.route.link = function(vnode) {
  if (vnode.tag === m.route.link && vnode.state == null) { // the `state` is null at this point in the closure
    return {
      view(vnode) {
        return m('a', vnode.attrs, vnode.children)
      },
      oncreate: _link, // without the `state` check above, this would return a component rather than linkify
      onupdate: _link
    }
  }
  } else {
    // current impl
  }
}

That being said @barneycarroll's assumption that a function with a view property would be considered a POJO component isn't misguided given the current semantics. Closure components as currently implemented may break existing code.

[pygy]

@barneycarroll I've modified the component kind detection code to treat functions with a view method as POJO components, not factories ( #1746).

[barneycarroll]

Great stuff @pygy :)

[isiahmeadows]

My event rework should fix this, since it now exclusively uses addEventListener and removeEventListener.