Mithril.js: Thinking about the default vnode.state value

[pygy]

The vnode factory currently always provides a state: {} object which is most of the time useless. @isiahmeadows has complained about it a few times but we kept it for the rare times where the hooks of non-component vnodes need a state.

We could do much better than this, though.

A simple plan would be to set it to null in the vnode factory and only add it in createNode for non-component vnodes. Recycled nodes would also need special treatment (there may be a bug left actually for recycled non-components anyway).

It would save a ton of allocations on redraw.

A more complex plan would rely on getters. With possibly two getters (to avoid a branch on every vnode.state):

function finalStateGetter(){return this._state}
function stubStateGetter(){
  this._state = {}
  Object.defineProperty(this, 'state', {get:finalStateGetter})
}

... where Vnode() would Object.defineProperty(vnode, 'get', stubStateGetter).

A similar modification would occur in initComponent.

The perf implications of the latter plan would have to be measured though. Hopefully redefining a getter would not change the shape of the object as far as JITs are concerned... It would save a larger chunk of memory than the simple plan, since vnode.state would only be created for non-component vnodes that have hooks.

Now that I think of it, it is possible that @isiahmeadows already proposed solutions along those lines but I didn't get them at the time...

[isiahmeadows]

@pygy I did propose this exact thing a while back, just not in any specific proposal.

Now that #1695 is merged, I'm going to go ahead and close this.

[pygy]

@isiahmeadows do you have any idea of the perf implications of adding getters to fetch the state (as proposed in the "more complex plan")?

[isiahmeadows]

@pygy I don't recall such a suggestion, and to be honest, properties > getters 99% of the time. (Less indirection)

[pygy]

@isiahmeadows It is literally the second part of the first post here, starting with "A more complex plan"

[isiahmeadows]

@pygy Oh...I thought you were talking about my ideas in #1669...

But now that I see what you're talking about, that idea would likely be slower initially, because you're modifying the object's internal map once (in Chrome at least - I think Firefox is similar, though), and Object.defineProperty is slloooowwww compared to basic property setting. Second, the indirection I'm not entirely convinced it's necessary. Third, you still have to address setters, because vnode.state = {foo: bar} is a fairly common thing to do - not everyone uses the immutable this in their hooks.

Or in summary, it's likely to be slower and more memory intensive, but for other reasons.

[pygy]

@isiahmeadows vnode.state = {} will cause issues with the next release (since the view and hooks are looked up on the state).

More generally, the addition of reserved fields on the state may be problematic for some (@ArthurClemens has reported problems in the chat, for example).

Do we consider this a Semver major break? @tivac @lhorie

[tivac]

Maybe? A lot of the subtleties of the vnode structure fly right over my head since I don't spend a lot of time in that system or do complicated things with components.

I don't think @lhorie is super into the idea of semver-major changes for the lib, though I've got zero problem with strictly following semver. Version numbers are cheap, after all.

[orbitbot]

Semver- _ish_ is probably a reasonable approach, however whatever practice applies should be specified in writing so users can figure out what to expect with version number changes if the versioning scheme deviates. Semver has a well-specified rationale, and deviating it without specifying what applies does not reflect well on anyone. As is right now, there are a few m.request issues floating around that would warrant major release number bumping on their own.

[tivac]

@orbitbot agreed, we're still in this awkward phase of "how do we not depend on Leo for literally every decision", and it's going to take a concerted effort to figure out things like this for the future.

[isiahmeadows]

@pygy

vnode.state = {} will cause issues with the next release (since the view and hooks are looked up on the state).

In practice, only outside oninit.

[pygy]

@isiahmeadows Everywhere, it will remove the view and hooks.

[isiahmeadows]

@pygy Oh yeah...I forgot that it trashes the prototype chain...

That's a breaking change I didn't even notice, something the docs need to be updated to reflect. 😟

I've filed #1727 in response.

[isiahmeadows]

Re-opening for consideration in 2.0.

[isiahmeadows]

Closing this as per subsequent discussion in Gitter and elsewhere - I doubt we'll ever implement this. First, we now do state: undefined in the vnode factory, creating the object later (even in v1), which alleviates the performance concerns. Second, it's unclear what this would even give us.