Make the first argument supplied to lifecycle arguments persistent

[barneycarroll]

The current component vnode API makes it easy to get stuck with stale vnode references, which becomes problematic if, for instance, you define persistent methods depending on vnode.attrs in oninit. The reduced failure case looks like this:

const Component = {
  oninit( vnode ){
    vnode.state.method = () =>
      alert( vnode.attrs.input )
  },

  view : vnode =>
    m( 'button', {
      onclick : () =>
        vnode.state.method()
    }, 'Feedback' )
}

m.mount( document.body, {
  oninit(){
    vnode.state.value = 'foo'
  },

  view : vnode => [
    m( 'input', {
      value : vnode.state.value,
      oninput : e =>
        vnode.state.value = e.target.value
    } ),

    m( Component, {
      value : vnode.state.value
    } )
  ]
} }

In the example above, the intention is to have the value of the input bound to the top level component state and passed in to the nested component, such that hitting the button alerts the input value. In practice, the alerted value will always be the same, because the method references the outdated vnode supplied on the first render.

This isn't a blocking problem — there are all manner of ways of getting around it. The problem is that all the intuitive patterns for defining custom stateful methods in components are liable to suffer from stale references to attrs.

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. Thus vnode.attrs is reassigned on every upstream draw, and the problem disappears.

[dead-claudia]

@barneycarroll You've got two issues in your example, which makes it harder to follow:

1. Your base oninit is missing the vnode argument.
2. Either your Component should alert with vnode.attrs.value, or you should pass an input argument when you use it in the base component.

As for the attributes, it is rather surprising that it doesn't mutate the retained vnode.attrs on each change. More specifically, it sounds like a bug to me. A more minimal (untested) example would be this:

var Component = {
    view: vnode => m("div", ["Count: ", vnode.attrs.count]),
}

m.mount(document.body, {
    count: 1,
    view: vnode => m.fragment([
        m("button", {onclick() { vnode.state.count++ }}, "Increment"),
        m(Component, {count: vnode.state.count}),
    ]),
})

[bruce-one]

@isiahmeadows that example works fine: https://jsfiddle.net/bruce_one/3w068s1j/

I think I hit something very very similar (although I can't quite remember in what exact incarnation) that was very frustrating/confusing initially. (Will try and remember how/why...) But it definitely came down to changes applying to the old vnode.state whilst the new vnode.state kept getting reinitialised (or not set, or some such).

I'd be keen on the idea of making vnode persistent to avoid "surprises". (vnode seems like it's consistent, so it's really surprising when something goes wrong -- and hard to find (because it's quite internal, imo))

[bruce-one]

Fiddle of @barneycarroll's code, just if of any use: https://jsfiddle.net/bruce_one/1yk6xchy/

[tivac]

This looks to me like standard JS object/non-object references, maybe I'm misunderstanding.

https://jsbin.com/haxosa/3/edit?js,console,output

var obj = { prop : 0 };
var ref = { prop : obj.prop };

obj.prop += 5;

// Because ref.prop is a reference to the value in obj.prop, when you update
// obj.prop ref.prop DOES NOT CHANGE
//
// This looks like exactly what is happening above in the mithril code

obj.prop // 5
ref.prop // 0, it still points at the old 0 value that obj.prop was previously

[pygy]

@tivac, yes and since on redraw the hooks/view receive a fresh vnode, the reference becomes stale.

IIUC, @barneycarroll suggests that we keep around and recycle the same vnode object for every redraw, with updated fields.

[bruce-one]

Initially when I ran into issues with this I thought about raising an issue, but couldn't isolate it well enough to do so... Hence I'm not sure I'm much help... But...

I'm fairly sure one of the reasons I ran into this is because I thought I could treat vnode.state as an immutable state container (ish).

Eg:

vnode.state = Object.freeze(Object.assign({}, vnode.state, { count: vnode.state.count + 1 }))

Personally, I was very surprised/confused when it didn't work (iirc, it was a bit race-y because it worked sometimes but not others? (iirc, integrating into non-mithril event listeners)) -- but now I think that I was misinterpreting the word state. (Had been doing some React, hence why I think I wanted to do it as such.) And hence understand if that type of use is not supported.

More recently I believe I've had issues with passing vnode to closures eg in oncreate which close over the original vnode value and hence the attrs have gone out of date (which I now understand/expect but initially it came as a surprise because I assumed that vnode.attrs was mutated not replaced). Although I can't think of a good example... So maybe ignore me on this :-s (Maybe https://jsfiddle.net/bruce_one/cmfL3gcc/ - click show then hide; and the vnode.attrs.onclick helps illustrate? (because in normal js, I'd expect the vnode.attrs.onclick value to have changed because of the object being updated)

[barneycarroll]

@tivac not quite. Your issue is with primitives being copied by value, not reference. The problem with the vnode object is that vnode and vnode.attrs are disposable values that never update. It doesn't matter if the properties you pass to attrs are primitive or not - if you define a method referencing vnode,attrs in oninit, and call that reference after a subsequent redraw, the reference will be to the attributes passed in on the first draw. This is a really nasty gotcha, because state does update, and Mithril's API encourages people to define stateful methods.

---

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. vnodes being disposable makes sense, the lifecycle methods make sense. the thing that's going against the grain is stateful methods, which authors should be dissuaded from under the component docs anti-patterns section.

[leeoniya]

seems like oninit( vnode ) should instead be oninit( instance ) with a instance.vnode that would get replaced, so your methods simply use that rather than the vnode directly. this is actually how domvm manages the same situation because its vnodes also get replaced (vm.node where vm is the referentially persistent component instance)

[pygy]

So you'd reserve the state.vnode field for the framework and swap vnode.state for state.vnode?

[leeoniya]

right, i imagine you'd have to either pass in state with reserved vnode or bind some other persistent wrapper to this within the hooks and access state.vnode or this.vnode. i'm not familiar with mithril internals, so just going off an educated guess from own experience.

[dead-claudia]

@bruce-one This might be better:

var Component = {
    oninit(vnode) { vnode.state.get = () => vnode.attrs.count },
    view: vnode => m("div", ["Count: ", vnode.state.get()]),
}

m.mount(document.body, {
    count: 1,
    view: vnode => m.fragment([
        m("button", {onclick() { vnode.state.count++ }}, "Increment"),
        m(Component, {count: vnode.state.count}),
    ]),
})

[dead-claudia]

I think it's that the new vnode, not the old one, is what's being passed to the view. An easy way to test this is using this:

var Component = {
    oninit(vnode) { vnode.state.vnode = vnode },
    view: vnode => m("div", "Same: " + (vnode.state.vnode === vnode)),
}

m.mount(document.body, {
    count: 1,
    view: vnode => m.fragment([
        m("button", {onclick() { m.redraw() }}, "Redraw"),
        m(Component),
    ]),
})

In order to maintain the correct attrs/state, the vnode.state has to be transferred each time to the new vnode instead of the new vnode.attrs to the old vnode. So I suspect this is what's happening, and may need fixed.

[leeoniya]

In order to maintain the correct attrs/state, the vnode.state has to be transferred each time to the new vnode instead of the new vnode.attrs to the old vnode. So I suspect this is what's happening, and may need fixed.

yes, this.

[mglazer-cengage]

Actually I don't see this as a bug at all and in fact as exactly the expected behavior.

The issue it seems is how the attrs are being stored or used by a child container not their value at any given point.

Firstly, attrs/props are the domain of the containing caller not the child component that receives them.

Secondly, If the child is trying to maintain them especially within an oninit event I would expect the exact behavior that is being shown.

That is, where only the oninit attrs are shown not as a reference or the latest but as it was when it was in fact called - oninit!

[leeoniya]

@mglazer-cengage

Focusing on oninit and vnode is perhaps not illustrating the issue adequately. The problem is being able to define handlers (whenever) that can access some persistent state, whether it be a transfered vnode.state, a bound this somewhere, or some other mechanism. The fact that state lives on the vnode is simply a means to an end given the current API. Unless i'm missing something.

[lhorie]

the vnode.state has to be transferred each time to the new vnode

It does this already: https://github.com/lhorie/mithril.js/blob/rewrite/render/render.js#L208

As I understand, the issue is when there's a component with a closure like in doStuff below:

// example 1
{
  oninit(vnode) {
    this.doStuff = () => console.log(vnode.attrs.count)
  },
  view() {
    return m("button", {onclick: this.doStuff})
  }
}

The code is written like this as an attempt to get a handle on the latest vnode.attrs.count, but clicking on the button logs the count from creation time, because the developer misunderstands how closures work.

To achieve what we want, the code needs to be written like this instead:

// example 2
{
  doStuff(vnode) {console.log(vnode.attrs.count)},
  view(vnode) {
    return m("button", {onclick: () => this.doStuff(vnode)}
  }
}

One proposal says example 1 should mutate vnode.attrs after the fact so that calling doStuff logs the latest count. Pros: it makes the actual behavior be in line w/ the developer's (flawed) expectations. Cons: this is voodoo magic

Another proposal is to have a vnode.state.attrs that always points to the latest attrs. Pros: This can be easily implemented. Cons: this complicates vnode consumption (i.e. it's usually more convenient to use vnode.attrs, but that doesn't work as per (flawed) expectation when closures are involved.

React gets around the whole thing by using a variation of the second proposal:

React.createClass({
  componentWillMount() {
    this.doStuff = () => console.log(this.props.count) // `this` is equivalent to `vnode.state`
  },
  render: function() {
    return <button onClick={this.doStuff}>;
  }
})
//or
React.createClass({
  doStuff() {console.log(this.props.count)},
  render: function() {
    return <button onClick={this.doStuff}>;
  }
})

[brlewis]

I think it's reasonable to tell people to write like example 2. 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.

[brlewis]

And if the reason they're writing example 1 is that they don't know how closures work, I woudn't add code to mithril's codebase that robs them of the opportunity to gain that understanding.

[bruce-one]

I entirely understand why example two is the solution, there's just something about the way the vnode is presented (or maybe/probably just how I read it?) that makes it feel like:

// Magic behind the scenes (_in my mind_):
var vnode = new ...

Component.oninit.call(vnode.state, vnode)
Component.view.call(vnode.state, vnode)
Component.oncreate.call(vnode.state, vnode)

// m.render()

Component.view.call(vnode.state, vnode)
Component.onupdate.call(vnode.state, vnode)

//etc (as opposed to thinking it's like `Component.view.call(state, { attrs, state, dom })` ish)

Hence in my mind it's just a standard reference argument, and hence reference to vnode.attrs would always refer to the new version (also, in my mind, that's what I'd want so it makes it feel even more likely/logical -- eg it seems like a trap to ever refer to vnode.attrs in oninit/oncreate because it's potentially/probably out of date).

I just feel like it's a bit of a trap and a bit confusing (when it bites you the first time; but from then on it's what you expect so it's fine).

Maybe the solution to my concern is just an equivalent of Typically, Virtual DOM trees are then recreated every render cycle, which normally occurs in response to event handlers or to data changes front and center in the lifecycle doc?

Although, in terms of oncreate and third-party-lib integration, there's now a bit of clumsiness (ime) eg:

const X = {
    view({ attrs, state }) {
        state.onclick = attrs.onclick
        state.onmouseup = attrs.onmouseup
        // ...
    }
    , oncreate({ state, dom }) {
        new ThirdPartyLib(dom, { onclick: (...args) => state.onclick(...args), onmouseup: (...args) => state.onmouseup(...args) })
        // because if this fn refers to attrs, and then there's a reason to change an attr, it won't work (feels trap-y)
    }
}

m(X, { onclick: state.create ? state.doCreate : state.xyz, ...

Anyway, I'm happy for this issue to be closed :-) (I just thought it a trap when I first encountered it, so felt it worth mentioning, but it's np :-) )

[lhorie]

@brlewis I think you have a very strong argument. I'm going to leave it as is