Make latest attrs available to event callbacks

[bdpartridge]

I really like Inferno's linkEvent(...) helper function (see doc), and I'm hoping that others also have interest in seeing a similar function get added to mithril.

It just feels bad/strange to have to recreate anonymous/arrow functions (or use function binding) to enable passing data from a component to event callbacks. Adding first-class support for a utility function like this provides an alternative that some may find cleaner. As a bonus, the Inferno doc claims a performance boost from this approach. I haven't verified the performance difference, but given the nature of closures and function binding, it makes sense.

Given the mithril doc's recommendation to "avoid fat components", a utility function like this seems like a natural fit since it helps to enable keeping state manipulation code separate from components. How about adding a similar helper function to mithril? Is there any interest? If so, I'd be happy to start working on it.

[pygy]

A solution to this would be to pass the vnode as a second argument to event handlers, as suggested by @chebum (here: #1739 (comment) it could be done easily)... Too tired for thinking of drawbacks right now...

[spacejack]

This should be doable with the next release which includes factory components:

const component = function() {
	let count = 0

	function increment() {
		count++
	}
	
	function view() {
		return [
			m('p', 'count: ' + count),
			m('button', {onclick: increment}, 'Increment')
		]
	}

	return {view}
}

m.mount(document.body, component)

demo

[leeoniya]

you guys might end up with domvm's API [1], including ES6 classes ;)

[1] https://github.com/leeoniya/domvm#views

[chebum]

@spacejack shouldn't Mithril perform state comparison between view method calls? Reading docs I thought that it checks if the state changed and then redraws the DOM. How does it work in reality?

[pygy]

@chebum Mithril diffs the virtual DOM trees (a.k.a. trees of vnode objects). The vnode.state field is persisted in the tree from one redraw to the next (it is transferred, for each node, from the old vnode to the new one), but isn't really needed with closure components (previously referred to as "factory components" but the term was deemed confusing).

Edit: Specifically, the engine diffs the tag, attrs and children fields of vnode objects. The other vnode fields are there for inner bookkeeping.

[barneycarroll]

@spacejack the closure component solves the issue of simple references to state; the linkEvent pattern allows you to reference anything available to the view (eg latest attributes).

I can only assume the performance benefit that differentiates this from simple partial application or any other kind of function declaration is special diff logic which allows the render loop to gloss over differences in linkEvent handlers and pick up the task of applying the function as part of Inferno's event bubbling system.

@bdpartridge reading between the lines I get the impression your main draw from linkEvent is a formal contract for binding events in views (as opposed to freeform functions). Mithril kinda offers this, albeit in a much more focused way: m.withAttr is a function for DOM event bindings that ensures a specified property of the DOM element will be fed to a specified function when the event triggers. It's especially useful in combination with stream. Ironically this was the prescribed method for handling events in Mithril v0.X, but Mithril v1 decided the scope of any given event handler was too large and the formality of invoking extra APIs to achieve simple assignment wasn't worth it. In v0.X, streams were simple getter / setters called m.prop — the old documentation for m.withAttr might prove insightful if you're looking for ways to standardise your event handling code.

As regards implementing linkEvent in Mithril… I think you could only really justify it in terms of performance gains. There are problems in Mithril components WRT expectations of what input is available to any given function, and you're not the only person to prefer documentedLibraryAPI(prescribedSignature) to myFunction(theArgumentsIWantToGiveIt).

I think it'd be good to hash out a (tediously long) document replete with code examples to cover all the aesthetic preferences in combination with all the practical motivations for function bindings in Mithril components. This would at least help us avoid going round the houses trying to remember preferred patterns every time this kinda thing comes up. 🤔

@leeoniya slowly slowly… ;) I was surprised you didn't mention the DOMVM architecture pattern in #1716.

[leeoniya]

@barneycarroll

Since Mithril's 1.x API is now baked, I think a philosophical discussion is rather moot, plus adding extra signatures and nuances to what exists leads to jquery-level of docs, user confusion and a lot of internal complexity with likely negative perf implications. I try to keep my trolling lighthearted and down to a minimum here; feel free to incorporate any of domvm's ideas.

If anyone still remembers 2015 (600 BC in javascript years), it was Mithril 0.2x's strange "controller", "this" and MVC concepts [1] that led me down my own path. Thankfully that's all behind us 1245 issues later :) It's good to see the Controller finally dead and the speed vastly improved. However even with 1.x, I still notice occasional mithril-isms carried over from 0.2, which is understandable but still not my cup of tea, especially given that I ended up writing exactly the lib i wanted to use.

[1] #499

[spacejack]

@chebum to answer your question about redraws, changes in state (vnode.state or otherwise) will not trigger redraws. Redraws are only triggered by events that are added to elements via attrs and when m.requests complete. Surprisingly (at least to me when I started using mithril) this covers most cases when you want redraws to happen. For cases that mithril doesn't detect, there is m.redraw().

[bdpartridge]

Thanks for the comments.

@pygy I like the idea of passing vnode as the second argument to event callbacks. It's simple and easy to document. Since I really just need access to vnode.state and vnode.attrs, this solution could work.

@spacejack with factory components, how would I access vnode.attrs in a nested component without creating a new function in view(...)?

@barneycarroll yes, I'm specifically interested in getting access to vnode.attrs (and vnode.state). But, I should've been clearer about the main thing that appeals to me about the linkEvent(...) approach. While I think it looks cleaner to not use .bind(this) or arrow functions, the main appeal for me is the potential for performance improvement (arguably unproven). We're using mithril v0.2.x in a fairly large production app at my current employer, and we've avoided recreating callback functions on every render because it seems needlessly wasteful. Maybe it is a micro-optimization, but I would still argue for the ergonomics of providing a utility function or just passing in vnode as a second argument to the callback, as @pygy suggested.

[dead-claudia]

I like best @pygy's proposal of just passing vnode as a second argument. That would fix this with no practical performance overhead, and it doesn't have the API boilerplate issues that Inferno's solution does (you don't have to explicitly call linkEvent to get the same benefits).

[pygy]

Giving credit where it's due, the idea comes from @chebum!

[cavemansspa]

nice. that's a much simpler approach to an issue for consideration a while back -- #1484 (comment).

[dead-claudia]

@cavemansspa The nicer thing about it is that we wouldn't have to add anything extra. You could just do it and access the config via vnode.attrs.config 😄

[vladpazych]

I am not sure if this is a bad practice, but what do you think about this:

// Main class to set vnode property
class MainClass<A> {
  private _vnode: m.CVnodeDOM<A>
  get vnode() {
    return this._vnode
  }
  render = (vnode: m.CVnodeDOM<A>) => { return m('span') }

  view(vnode: m.CVnodeDOM<A>) {
    this._vnode = vnode
    return this.render(vnode)
  }
}

// And after that - just beautiful syntax with vnode accessible everywhere
class TestButton extends MainClass<{ value: number, onclick: Function }> {
  onclick = () => {
    // Extend behaviour, or override
    if (this.vnode.attrs.onclick) this.vnode.attrs.onclick()
    console.log(this.vnode.attrs.value)
  }
  render = () => {
    return m('button', { ...this.vnode.attrs, onclick: this.onclick }, this.vnode.attrs.value)
  }
}

[dead-claudia]

@vladpazych It's an equivalent workaround, but it'd be better to just pass a second argument.

[dead-claudia]

Implemented @chebum's idea of passing vnode as a second argument here.

[dead-claudia]

Welcome!

[JAForbes]

A little late here (sorry), but passing the vnode as a second arg seems like a bad idea to me. We're making mithril event handlers proprietary and I disagree that adding an additional argument isn't a major semver release, I have code that will throw with this change.

This reminds me of passing the index in Array::map, it seems convenient, but it actually discourages composition and leads to sometimes surprising behaviour.

To illustrate my point, here's an example of passing additional args radically changing behaviour.

R.map( R.slice, range(0,10) ) has very different behaviour to R.range(0,10).map( R.slice ), the first creates a list of functions that slice from a particular index, the second fully evaluates slice because the index, and the list are provided to the Array::map visitor function.

R.range(0,3).map( R.slice ) //=> [[],[],[]]

R.map( R.slice, R.range(0, 3)) //=> [R.slice(0), R.slice(1), R.slice(2)]

I also don't see what problem this solves, vnode is always going to be in scope, so we're diverging from mithril's thin wrapper over the DOM API for little benefit. This notion of avoiding oninit/controller by spreading coupled logic into various hooks and event handlers mystifies me. On the quest to avoid "fat controllers" we end up with spaghetti code, in denial of its truly coupled nature.

I can mitigate this change with a helper that forces the function to be unary, but its awkward, and I wanted to voice my objection. I expect this will be a change we end up regretting later, like prop's having a .then method. It seems convenient, but it is surprising and makes mithril more complex.

If this improves performance, we should have benchmarks before making decisions about the value it adds. It may be in the noise, and unless its a significant boost I don't agree its a valid trade off. Ironically, if one wants to avoid that second argument we're forced to create an intermediate function, which is the exact problem this feature is meant to solve.

[leeoniya]

I also don't see what problem this solves, vnode is always going to be in scope

i'm no mithril expert, but it seems that only the component's root vnode would be in scope, but not the vnode of a handler defined deeper in the returned vtree. right?

or do you guys hang the current vnode off e, e.target or this?

[2is10]

At the risk of offending my coworker, @bdpartridge, who filed this issue, I agree with @JAForbes. Encroaching on a DOM API has a bit of a code smell.

Factory components seem to solve the problem, at least for the common case.

@spacejack the closure component solves the issue of simple references to state; the linkEvent pattern allows you to reference anything available to the view (eg latest attributes).

@barneycarroll, accessing the latest state or attrs is easily solved by stashing a reference in a component-scoped var each time view is called.

@spacejack with factory components, how would I access vnode.attrs in a nested component without creating a new function in view(...)?

@bdpartridge, I’d love to see a code sample illustrating the nested component situation you’re describing. Is it common?

[JAForbes]

Could someone clarify what they mean by "stale attrs". I'm still on 0.2x day to day so apologies if I misunderstand something, but aren't attrs bound at component creation time, and aren't updated every redraw?

var root = document.body;

function Parent(){
    return {
        view(){
            return m('div'
                ,m(Child, { value: Math.random() })
                ,m('button', { onclick: i => i }, 'Redraw')
            )
        }
    }
}

function Child(){
    return {
        view(vnode){
            return m('p', JSON.stringify(vnode.attrs) )
        }
    }
}
m.render(root, m(Parent));

In the above example, my understanding is, we'll always render the same random number, no matter how many times we redraw, even though the Parent component appears to be passing a new random number on every render.

That's my understanding. That said, receiving vnode.attrs fresh every render would be a great thing in my opinion, it would solve a lot of problems I have with stale components that need to be instantiated to receive new data, I currently solve that by either using a stream that is passed down, or by using a plain old function (where possible).

So is my understanding wrong? I just tried the above code in a bin and it seems to follow my expectations. Could someone elaborate what "stale attrs" means in their context?

[spacejack]

Nope, you get updated attrs every redraw: http://jsbin.com/caxosogeqe/edit?html,js,output

[JAForbes]

@spacejack oh wow! 🎉

[JAForbes]

Just found out why my example code failed, the template was using m.render instead of m.mount 😮

[JAForbes]

But yeah as @spacejack's bin shows, vnode is in scope, we get the fresh value, and I still oppose this change.

[cavemansspa]

@JAForbes -- i recall a a related conversation on vnode here: #1521

[dwaltrip]

React's solution to this problem is to have this.props always point to the latest values that were passed into the component, where this is a persistent object representing the component instance.

[JAForbes]

A summary of the thread @cavemansspa referenced for consideration.

@lhorie:

Pros: it makes the actual behavior be in line w/ the developer's (flawed) expectations. Cons: this is voodoo magic

Link

@barneycarroll:

I'm withdrawing the proposal to make component vnodes persistent objects because that treads on the toes of the update methods, which rely on newVnode, oldVnode signatures.

Link

@brlewis:

Making changes to the mithril codebase in order to hide the fact that vnodes don't persist through the entire lifecycle won't work. People will still encounter confusion, only later and in more subtle ways.

Link

[JAForbes]

I don't have enough experience using 1.0 to feel comfortable challenging any of the above arguments from the other thread. I still oppose an extra argument for event handlers and it does seem like a smell to me to have event handlers accessing mithril specific properties on an event.

I personally think that currying the handler is a great solution, and worrying about performance for creation of functions is untested and probably misguided - but I can understand the frustration of having to do that every time, it feels like something that should be supported somehow in the default API without any trickery.

If anything closure components are more susceptible to this particular annoyance. I think when I eventually migrate to 1.0 I'd decorate the component interface so I get a stream of "fresh" attrs that I can subscribe to and merge with other event streams in the closure. But streams aren't part of the core library so that isn't a solution everyone else is likely to want to adopt.

Of the three proposed solutions, I don't think there is a satisfying addition, but adding a property to the event seems like the path of least resistance, so if we have no other options and we want to make some kind of change then I'd reluctantly support that.

As @spacejack has stated We're already modifying the event object to have a redraw=false property, so from that perspective its consistent. If we're modelling mithril event handlers in typescript or flow it's already going to be an intersection type.

But if someone has a different solution I'd welcome that 😄

[2is10]

To be clear, what @dwaltrip suggested is different from what was proposed in that other thread.

@dwaltrip (this thread):

What if instead of passing in vnode everywhere, we passed a persistent object representing the component instance, and every render loop this object is updated with fresh references to the vnode, attrs, state before passed to any lifecycle methods?

@barneycarroll (other thread):

We can mitigate this by changing render logic such that the vnode received by components — which need not necessarily be the same construct used by the render internals — be stateful, this ensuring that the vnode received by any given component instance will be the same throughout that component's perpetuity.

In addition, that other thread predates closure components. @lhorie’s harsh criticism, in particular, was about the idea of mutating the vnode passed to oninit for a “developer who misundertands how closures work”.

The closure component API might have benefitted from a little more thought about how closures work. That new API would have been a natural place to introduce a new object type—one whose properties are always up-to-date:

const ChildComponent = function (o) { 
  // Use o.vnode, o.attrs, o.state (each always up-to-date).
}

I’ve shown it as o above as a pictogram of the One ring and as a symbol of renewal. Also has the nice effect of making each property access sound poetic. :)

Or could’ve passed a latest-vnode getter instead of just the initial vnode:

const ChildComponent = function (getVnode) { 
  // Use getVnode().attrs for up-to-date attrs.
}

[2is10]

While I like @dwaltrip’s suggestion and the event.vnode suggestion, here’s one more concrete, easy (?), small, backwards-compatible possibility to consider: Pass a latest-attrs getter as an additional arg to closure components. People could name it however they please (e.g. getLatestAttrs, getAttrs, attrs).

const ChildComponent = function (vnode, attrs) {
  ...  
  function onclick() {
    showSuccess = true;
    setTimeout(() => {
      showSuccess = false;
      m.redraw();
    }, attrs().showSuccessTime);
  }
  ...
}

[dead-claudia]

@JAForbes As someone who has done significant hacking on the renderer itself, the difficulties of making the vnode persistent is generally overstated. The main requirement is that you understand the renderer algorithm (which only a few of us do), but beyond that, it's mostly name changes, and it's something you can generally do for components and DOM vnodes separately. It's more tedious than it is hard.

[spacejack]

One more point for having some way to access to the current vnode in an event handler is getting the current dom (which isn't available in the vnode provided to view.)

[dead-claudia]

@spacejack It's inadvisable to access vnode.dom in the view, anyways - it's not there during the first render, and it doesn't see the updates from the current render (and is likely to get interfered with).

[spacejack]

I mean in an onclick etc. handler.

[pygy]

... getting the current dom (which isn't available in the vnode provided to view.)

@spacejack actually, the vnode is the same in all hooks of a given redraw() cycle, it is mutated after the view returns. @isiahmeadows I don't think it ever has the .dom set until the view returns...

[spacejack]

I suppose you can get the clicked element via event.currentTarget (maybe need to crawl through the dom for the element you want) so that's not so bad. However there's this case:

const c = {
    view() {
        m(comp, {
            onFoo: () => { /* want to use dom here */ }
        })
    }
}

EDIT: Sorry my brain isn't working today... that's not a mithril event handler duh. I suppose the dom access is mostly a non-issue.

[dwaltrip]

@spacejack This is what you meant? Looks like it could cause issues as you were suggesting.
Although this approach could be somewhat inherently precarious, I can imagine a few situations in which it might make sense.
I've found that these edge cases come up more frequently when you are doing things with 3rd party libraries. Of course, there are work arounds, but it usually involves a bit of extra code & manual bookkeeping.

const FooComponent = {
    view(fooVnode) {
        return m('.foo-div', m(BazComponent, {
            handleSomething: () => { /* want to use FooComponent dom. using fooVnode won't work */ }
        }));
    }
}

[dead-claudia]

@pygy I think you missed this part 😉

[...] it's not there during the first render, [...]

[pygy]

@isiahmeadows It's not there in view during subsequent render() either, not on vnode. old in onbeforeupdate(vnode, old){} has .dom set, and it is present in oncreate/onupdate, off course.

[dead-claudia]

Oh okay.

[pdfernhout]

I agree that always adding the vnode to the event handler call feels problematical.

The point here is to find a way that event handler functions can get extra information they need, right? And without breaking existing code?

The linkEvent idea seems good to me to accomplish that and the code does not look that complex:
https://github.com/infernojs/inferno/blob/7a27b8ff2a8b03f68a0091cf790c1d78aad57c43/packages/inferno/src/DOM/patching.ts#L825

Another variation along the lines of linkEvent is to optionally pass extra parameters to the called event handler function based on defining some new property of attrs like "bind" or "extraArgs" or whatever. Such an approach might also be useful to selectively bind "this" as well.

Thinking about it, it does not seem as general as linkEvent though. The linkEvent approach could even perhaps be expanded to return a boolean that controls whether to do a redraw?

Adding a field to the event seems reasonable to me as an alternative too.

With either optional approach, if users want the extra parameters, they get them if they set an extra field in attrs. If they don't want them, things work as-is so existing code will not break. Maybe there could even be a flag to pass the vnode in to the event handler too if that was really desirable?

Either idea avoids the need to to create a new closure with bind or an anonymous or curried function -- which can have performance and garbage collection implications depending on whether that is done in the view function or now.

I proposed a variant of a similar approach for Maquette where, ultimately, you could set a "bind" property for "this" to make it easier to call instance methods for TypeScript classes without extra closures. Here is the issue which was resolved with such a change: AFASSoftware/maquette#35

Of course, one can question if the extra complexity is worth it given there are these possible workarounds for where performance matters. There would also be a small overhead all the time for extra checks if such fields were defined. But there is something to say for supporting a more idiomatic object-oriented use even if Mithril encourages a more functional coding style -- and for that, being able to easily bind "this" is important (which modifying the event does not address).

@spacejack pointed me at this conversation from Gitter discussion about possibly changing Mithril to use a constant stub function which calls user functions rather than create a new wrapper function and touch the DOM ever time the user event handler changes (like if a bind call or anonymous new function is used in defining the attrs). Supporting optional parameters or binding this as desired might reduce the likelihood people would write such code and tickle this possibly computationally inefficient aspect of Mithril. Again though, the alternative is restructuring user code when computational efficiency is needed -- and also perhaps updating how Mithril handles event handler changes.

[dead-claudia]

@pdfernhout That stub would reduce the memory load, too. Oh, and event listeners don't have to be functions: they can be objects, too, provided they have a handleEvent method. (Almost nobody does this, but it is a thing we could use here.)

[dead-claudia]

That could allow us to just add vnode.events.vnode = vnode and vnode.events.handleEvent = handleEvent (both guaranteed to not conflict) when a handler is first created, and do vnode.dom.addEventListener(key.slice(2), vnode.events, false) directly on first set of each new event. The logic would likely look like this (with this line using addEvent and this line using removeEvent):

var handleEvent = typeof onevent === "function"
    ? function (e) {
        var type = "on" + e.type
        if (this[type] != null) this[type].call(this.vnode, e)
        onevent.call(this.vnode.dom, e)
    }
    : function (e) {
        var type = "on" + e.type
        if (this[type] != null) this[type].call(this.vnode, e)
    }

function addEvent(vnode, key, value) {
    if (vnode.events == null) vnode.events = {handleEvent: handleEvent, vnode: vnode}
    if (vnode.events[key] == null) vnode.dom.addEventListener(key.slice(2), vnode.events, false)
    vnode.events[key] = value
}

function removeEvent(vnode, key) {
    vnode.events[key] = undefined
    vnode.dom.removeEventListener(key.slice(2), vnode.events, false)
}

And in practice, addEvent would normally only change the value.

[gilbert]

I just ran into this problem yesterday on 1.x. I remember running into this problem in 0.x as well. I've never had to think about this problem when working with React, so this is one aspect it has over Mithril.

For the record, my use case is integrating with a 3rd party lib:

oncreate(vnode) {
  var editor = CodeMirror.fromTextArea(dom.children[1], ...)

  editor.on('mousedown',function (editor, e) {
    e.preventDefault()
    if ( vnode.attrs.mode === 'pending' ) {
      editor.focus()
      editor.setCursor(0,0)
    }
  })
}

I like @2is10's proposal.

[dead-claudia]

@gilbert I could've used this feature, too, in multiple cases.

Now that I have taken a bit off, I have an idea that might work a little better:

1. vnode.attrs and vnode.children are always the latest version, rather than the version at the time of the call.
2. The switch would happen between onbeforeupdate and view.
3. onbeforeupdate would change to be called as vnode.state.onbeforeupdate(vnode, next), so we don't have to swap attrs/children on the two vnodes before calling, and then swapping them again.

It would both simplify internals some and resolve this bug. Also, it wouldn't require us breaking the world to do so (few components in practice depend on the current behavior apart from the onbeforeupate parameter order), so it would be easier to migrate.

[gilbert]

vnode.attrs and vnode.children are always the latest version

Sounds wonderful! Would this mean vnode is always the same object? Not that I need it to be, I'm just curious.

[dead-claudia]

@gilbert Eventually, it could, but that's not an absolute requirement for my proposal.

[dead-claudia]

BTW, here's how you can work around it: set lastAttrs = vnode.attrs before returning the view and use a shared handler in a closure.

function Component() {
	var count = 0
	var attrs

	function increment(ev) {
		count += attrs.count || 1
	}

	return {view: function (vnode) {
		attrs = vnode.attrs
		return [
			m('p', 'Count: ', count),
			m('button', {onclick: increment}, 'Increment by ', attrs.count),
		]
	}}
}

You can do similar with classes and object components if you leverage the IMHO underused handleEvent:

class Component {
	constructor() {
		this.count = 0
		this.attrs = undefined
	}

	handleEvent(ev) {
		this.count += this.attrs.count || 1
	}

	view(vnode) {
		this.attrs = vnode.attrs
		return [
			m('p', 'Count: ', this.count),
			m('button', {onclick: this}, 'Increment by ', this.attrs.count),
		]
	}
}

var Component = {
	oninit: function () {
		this.count = 0
		this.attrs = undefined
	},

	handleEvent: function (ev) {
		this.count += this.attrs.count || 1
	},

	view(vnode) {
		return [
			m('p', 'Count: ', this.count),
			m('button', {onclick: this}, 'Increment by ', this.attrs.count),
		]
	},
}

And of course, this carries for literally any vnode property or even the vnode itself - it works the same way. So since you can basically just design your way out of the (almost never significant) performance issue by just using native stuff and existing functionality, I'm closing this.