Delegation to `vnode.state` for view methods break `vnode.state` assignment idiom

[dead-claudia]

This was initially brought up by @pygy, but somehow flew right under the radar until recently... 😟

TL;DR: this no longer works, but will instead throw internal errors:

var Component = {
    oninit(vnode) { vnode.state = {counter: 1} }
    view(vnode) {
        return m("div", [
            m("div", ["Count: " + vnode.state.counter]),
            m("button", {onclick() { vnode.state.counter++ }}, "Increment")
        ])
    }
}

m.mount(document.body, m(Component))

---

Where @pygy and I changed to using vnode.state instead of vnode.tag as the source of truth for component hooks, that meant the previous idiom of assigning your initial state to vnode.state no longer works now, because the prototype that contains the view and other hooks is now lost.

We'll have to document this as a breaking change in the changelog for v1.1, and we'll likely have to edit the documentation accordingly to not write to vnode.state where applicable and to document vnode.state as read-only/volatile (since Mithril sets it on its own).

[tivac]

🤔

Isn't the entire point of vnode.state that you can write to it? Making it read-only is a huge breaking change and I'm not ok with shipping that due to the churn it'll cause in even my moderately-sized 1.0 codebase.

[orbitbot]

@tivac IIUC this case only covers the case where you overwrite vnode.state completely (eg. assigning an object like above). Or otherwise most of the current usage flies out through the door with the rest of the bathwater... This is also on its own a major version bump level change.

[tivac]

Yeah & I still think that's bad, also not a very end-user friendly change IMO. I also don't think it is at all worth a breaking change based on my understanding of the underlying goal.

[pygy]

Without this, supporting class components becomes far more involved... Branches all over the place in render/render.js

[tivac]

Yet another reason for me to dislike classes as components then. This is a rather large footgun that seems like it'll be real painful to debug, I don't know that docs are going to cut it.

[pygy]

I'm sorry I didn't emphasize this in #1339. The implication was clear to me, but I should have made it clear that it was a footgun...

For the simple cases, migration is possible with a codemod... replace vnode.state = x with Object.assign(vnode.state, x). That doesn't cover the cases where you overwriting an old state with more properties than the new one though, and may be problematic if you were relying on object identity semantics (if (vnode.state === y){...}).

[dead-claudia]

Should we add a new field to vnode to hold the component instance in case vnode.state is overwritten? (It could also benefit factory components that do the same thing.)

[pygy]

Expanding on my previous comment, I knew that reserving the view and hooks fields would be breaking, but I didn't anticipate that replacing vnode.state would be considered and idiom, because it's always been somewhat broken: if you replace the state in a "before view" hook, the old value is still passed as this to the corresponding "after view" hook.

@isiahmeadows that was your original design for the feature, and it could work...

[dead-claudia]

@pygy

that was your original design for the feature, and it could work...

This is why I took that route initially, to avoid screwing with vnode.state. Just the resulting big diff of it all put together turned a few people off (including you).

[dead-claudia]

But I completely forgot about that over the few months it took for it to finally gain enough traction to have an accepted implementation.

[pygy]

Looking at master it looks like the oncreate context doesn't match vnode.state bug I described above isn't there, maybe I'm misremembering or mixing things up...

[barneycarroll]

vnode.state is a Mithril API — I think it's reasonable to assert that overwriting library-managed properties is generally a bad idea that will lead to unexpected behaviour.

If ever an authour finds themselves doing this, I would expect them either to be trying a really ambitious hack, in which case the insights above might come in useful depending on their intent, or simply trying to quickly assign a bunch of properties without repetition, in which case I would advise Object.assign instead of direct assignment.

If we get regular complaints, we might consider defining state such that set performs Object.assign under the hood.

But introducing the complexity of allowing vnode.state replacement without a use case is just a really bad idea IMO. Imagine having to caveat "vnode.state is the same reference as this in component methods, and is an instance of vnode.tag" with "except when it isn't"…

[pygy]

Actually, here's what's broken in master: Replacing vnode.state in the view isn't reflected in context of the corresponding post-view hook. oncreate and onupdate get the previous state instead.

[pygy]

Live here, with the current master: http://jsbin.com/zulapabeyu/edit?js,console

[pygy]

@barneycarroll Using Object.assign under the hood has the caveat I mentioned above... We can't freeze the vnode either because the engine relies on mutating various properties (skip, dom, events, plus recycling when the reuse patch lands). We could use a getter/setter pair to protect it (either with Object.assign or by throwing an error)... I already thought of using such a pair to lazily create the state on non-components vnodes, but @isiahmeadows thinks it would lead to poor perf...

[dead-claudia]

@pygy

I already thought of using such a pair to lazily create the state on non-components vnodes, but @isiahmeadows thinks it would lead to poor perf...

Could you explain where you're coming from? A single branch, even mispredicted, is way cheaper than a single allocation in the same code path, even when the GC'd object is pooled.

[barneycarroll]

Using Object.assign under the hood has the caveat I mentioned above...

Maybe I've misunderstood something crucial. This thread starts with the premise that the author wants to write vnode.state = {/*…*/}, and the rest is a discussion of the changes to core code necessary to allow that. My contention is that authors shouldn't replace vnode.state for the same reasons they shouldn't replace eg a jQuery object's data method.

To put it another way: if you don't overwrite library APIs in undocumented ways, you avoid an infinity of problems. It seems fundamentally wrong-headed to refactor Mithril to allow this pattern in some scenarios — especially in the absence of any kind of feature gain.

[dead-claudia]

@barneycarroll

It seems fundamentally wrong-headed to refactor Mithril to allow this pattern in some scenarios

To be fair, the required refactor would be minor. Not arguing whether even a minor refactor would be okay, though. I'm exceptionally neutral on this.

[pygy]

@isiahmeadows https://github.com/lhorie/mithril.js/issues/1694#issuecomment-285423127 actually, it's the defineProperty bit that you flagged as "sllloooooowww" ;-)

My contention is that authors shouldn't replace vnode.state for the same reasons they shouldn't replace eg a jQuery object's data method.

@barneycarroll agreed, since the pattern is already subtly broken, but people (including two core devs) actually do it, and the change breaks their apps...

If we only want to cater to the "overwriting vnode.state with an object literal that isn't referenced elsewhere" case, then Object.assign fits. If someone assigns an empty object to wipe the state, further down the lifecycle, it breaks. Likewise if someone is using a reference to the state object elsewhere and rely on object identity (const state = {x:5}; someSet.add(state); vnode.state = state). The former is quite unlikely, and the latter can be fixed (this.x = 5; someSet.add(this)) though.

[dead-claudia]

@pygy You can lazily set it based on whether the relevant magic properties exist, without resorting to defineProperty. Just getting it out of the common path of no attribute hooks at all is good enough. 😉

[tivac]

and to document vnode.state as read-only/volatile (since Mithril sets it on its own).

I misread the intent of this line, sorry for the confusion.

That said, setting vnode.state = {...} doesn't seem like an unreasonable thing to want to do. If we really want to break it my only request is that it be documented very carefully. I consider this change a potential footgun but won't personally be affected.

[spacejack]

As someone who didn't use 0.2 very much, since vnode.state comes from mithril, I wouldn't have assumed I could reassign that object myself. I didn't realize you ever could until this issue popped up actually.

[barneycarroll]

I'm surprised two core devs (@pygy & @isiahmeadows?) would want to do this in application code.

Something that's plagued Mithril on and off from day one is the ambiguous nature of how it treats 'classy' code, and my instinct is to avoid compounding the inherent vagaries of this & prototypal structures with special exceptions (this is vnode.state, which is an instance of vnode.tag… except when…). But maybe that's a high-handed paternalistic attitude.

I'm still bothered by the extra logic necessary to accommodate this (I'm going to say it!) aesthetic preference: isn't it weird to make core jump through so many hoops to accommodate this degree of mutability considerations? Surely mutating the vnode itself in app code is a Bad Idea in the absence of a compelling feature unlock?

Full disclosure: I was the person who screwed this up by removing the conditional logic in vnode update / creation with Object.create(tag).

[pygy]

That said, setting vnode.state = {...} doesn't seem like an unreasonable thing to want to do.

Agreed, it's a matter of affordance... JS objects are by default mutable and vnode.state is an invitation for replacement... Provided that it mostly works (it actually 100% works except in view, which is easy to miss) it is not surprising that you picked it as a pattern.

If it was a distinct parameter, or just this, the issue wouldn't have arisen.

@barneycarroll I don't want to do this, at all (you probably meant @tivac and @isiahmeadows). I didn't consider it as a thing that #1339 would break because it was already broken (in view) and a priori unsupported (the docs don't mention replacing the state, and the examples only add properties). Also IIRC the conditional logic in createComponent was to paper over the lack of a state object on vnodes that were not created by the Vnode factory (the tests are full of vnode literals that don't have a state field). If it was only me I'd probably either leave it as is or throw an error on set. @tivac seems to disagree though.

[tivac]

@pygy It's... ok. ¯\_(ツ)_/¯

Just needs calling out in the docs, IMO.

[dead-claudia]

@barneycarroll

I'm surprised two core devs (@pygy & @isiahmeadows?) would want to do this in application code.

I never do it myself, but I have only a mild opinion on what others do. To clarify, I initially suggested keeping the current logic and explaining the break, and that's my personal preference, but @tivac provided swift resistance.

isn't it weird to make core jump through so many hoops to accommodate this degree of mutability considerations?

Since @pygy and I decided to make vnode.state the sole source of truth to abstract over the differences, fixing this break is nearly trivial; 90% of the diff would be patching tests to initialize the extra field where applicable.

And from both here and Gitter, people felt very strongly for all three: you for objects, @JAForbes (and other FP fans) for factories, and @spacejack (and other TS users) for classes. That's why @pygy and I sought to add support for all three.

Full disclosure: I was the person who screwed this up by removing the conditional logic in vnode update / creation with Object.create(tag).

Could you find that commit, so I can study how semantics actually changed with it? Also, it might explain recent troubles with oncreate state having the wrong vnode.state when it's mutated.

[dead-claudia]

@barneycarroll Also, to be fair, the docs said nothing about replacing vnode.state, and we never really took into consideration much how it should be treated. The whole reason I filed this issue is because we've got to do something about this - it's going to inevitably trip people up, so we've got to determine whether to allow the usage or to create a codemod and warn people to stop doing it. I'm mildly in favor of the latter, but @tivac seems to prefer the former (and I'd suspect @lhorie might as well, but that's just an educated guess).

[pygy]

Could you find that commit, so I can study how semantics actually changed with it? Also, it might explain recent troubles with oncreate state having the wrong vnode.state when it's mutated.

I think it's unrelated. Since Vnode used to provide an object in the state field, to avoid another allocation, the shallow copy code was using that object, when present, as a target. When vnode.state was null, a new object was created.

oncreate has a different state because it is scheduled and bound in initlifecycle, just after oninit fires and before view does.

this in post-view hooks could be fixed by pushing both vnode.oncreate/vnode.onupdate and vnode in the hooks array, and iterate over hooks with a stride of 2 slots, and do hooks[i].call(hooks[i+1].state, hooks[i+1]). It would also save the allocations due to bind calls for the post-view hooks (but it would make the hooks array grow twice as fast).

Edit: re-ordered sentences so that the post makes some sense...

[dead-claudia]

@pygy

this in post-view hooks could be fixed by pushing both vnode.oncreate/vnode.onupdate and vnode in the hooks array, and iterate over hooks with a stride of 2 slots, and do hooks[i].call(hooks[i+1].state, hooks[i+1]). It would also save the allocations due to bind calls for the post-view hooks (but it would make the hooks array grow twice as fast).

You could achieve the same thing by pushing a {func, vnode} pair each time, and calling it via hook.func.call(hook.vnode.state, hook.vnode), getting the added benefit of simpler iteration and more flexible scheduling + call handling. Node already does similar for scheduled tasks in process.nextTick.

[tivac]

Is this going to be documented? I'm writing up the PR for the last bits of release process automation and I'd like to cut 1.1.0 on Monday.

We can probably ship that without this being documented but it needs to stay on the radar.

[pygy]

I could give it a go.

Note that the have already been updated, to a degree (state is now described as "provided by the engine when needed"), but I guess more would be needed. Especially mentioning that view and the hook names are now reserved. Technically one could still replace the state object, provided it has at least a view method...

Regardless of documentation, since we reserve names on the state for the engine, shouldn't this be v2.0?

[tivac]

Mithril has traditionally been semver-ish, and @isiahmeadows requested that we continue that until the new APIs stabilize some more.

I don't care about major versions at all and would happily consider class components and all the breakage they have wrought to require a 2.0. I haven't heard anything from @lhorie on the matter that I can recall.

[pygy]

Let's keep it semver-ish then... There are breaking changes looming for m.request as well IIRC (regarding 'POST' and url params interpolation).

[dead-claudia]

@tivac I suggested mitigating it by adding a vnode property to fix the immediate issue at hand, which should dock it to a minor release. We could then deprecate it for v2 so we can remove the indirection later.

(The renderer code needs a lot of cleanup anyways. I'm not sure we actually need to have a dozen vnode properties for the state, and some temporary state is stored in the vnode already. But some of those things are difficult to do without minor API modifications.)

[pygy]

@isiahmeadows done in #1746. Edit, which is now merged.