[rewrite] Big problem with object schizophrenia

[ciscoheat]

createComponent has a problem in that it creates a copy of the keys in vnode.tag, placing them in vnode.state, ignoring the prototype which may have methods required for displaying the view correctly.

vnode.instance = Vnode.normalize(view.call(vnode.state, vnode))

The above line prevents using vnode.tag, the actual object reference, as an object. When the view function is a method of the vnode.tag object, view will surprisingly be called with the vnode.state object, which looks similar to the vnode.tag object, but it isn't, so any prototype method calls in the view method will fail.

[barneycarroll]

Hi @ciscoheat, good to see you again!

This should probably be documented as a gotcha — I prefer that term to 'anti-pattern' as currently used in the docs, which has a tendency to inform dogma.

Mithril has always had an idiosyncratic attitude to JS objects and this, but I think it's acceptable for Mithril to lay down the requirements for the vnode object contract — it is after all a custom type of its own devising. This leads to less runtime ambiguity, no?

What are component prototypes doing for you in your current codebase @ciscoheat?

[dead-claudia]

IMHO, this is really a bug. The special hooks Mithril uses should be read up the prototype chain. Although, I'll point out that using Object.assign (or similar) instead of prototypes will alleviate this problem mostly.

[pygy]

@ciscoheat mea culpa... I initially used a for in loop that was visiting the prototype chain of traditional JS objects (and class instances transpiled with Babel and Bublé), but it turns out that native ES6 methods are not enumerable on instances, so I went for Object.keys.forEach(...) instead.

We could instead use vnode.state = Object.create(vnode.tag) since that function is supported by IE9.

I didn't care much about it initially because I thought that Class instances as components were a stopgap until classes as components were supported, but @lhorie delayed that until after v1, so it may be relevant again...

Edit: I started answering this earlier and missed @isiahmeadows' reply... Object.assign doesn't copies the methods of ES6 class instances (I think because of their non-enumerability).

[ciscoheat]

Hi guys, it's nice to be back. :) I'm taking the first steps toward making the rewrite work in Haxe.

@barneycarroll well, currently I cannot even do this:

function Numbers(numbers) {
    this.numbers = numbers
}

Numbers.prototype.odd = function() {
    return this.numbers.filter(function(n) { return n%2 == 1 })
}

Numbers.prototype.view = function() {
    return this.odd().map(function(n) { return m('p', n) })
}

var nums = new Numbers([1,2,3,4,5]);

m.mount(document.getElementById('content'), nums)

I would be quite disappointed if the rewrite denies me that fundamental OO notion...

@isiahmeadows and @pygy , I don't think any duplication of an object is a good solution. Identity is a fundamental part of objects as you know, losing that can lead to very subtle bugs.

My current quick-fix is to just put:

vnode.instance = Vnode.normalize(view.call(vnode.tag || vnode.state, vnode))

But of course, I have no idea if that works in general.

[ciscoheat]

Sorry for the error, fixed now.

[lhorie]

I don't think we can do without the object copying. Consider for example

function Numbers(numbers) {
    this.numbers = numbers
}

Numbers.prototype.oninit = function() {
    this.numbers = this.numbers.filter(function(n, i) { return i%2 == 1 })
}

Numbers.prototype.view = function() {
    return this.numbers.map(function(n) { return m('p', n) })
}

var nums = new Numbers([1,2,3,4,5]);

m.mount(document.body, {view: () => [m(nums), m(nums)]})

The state object must be different for each component, so vnode.tag can't pass as the state object for both, otherwise the example above would break.

The example above may seem dumb, but it's fairly idiomatic to do things like this:

things.map(thing => m(SomeComponent, {thing: thing}))

SomeComponent instances may have internal state and their states should not be shared with other instances

losing that can lead to very subtle bugs

I don't think you'd ever want to rely on equality between this (in a class method) and nums for the reason above.

[ciscoheat]

I don't mind if the state object exists and is copied from the original object. But what is the problem with having this referring to the actual object? Don't you find it confusing if this becomes something else in the prototype, especially something that looks very similar to the object created from it, but isn't?

[barneycarroll]

What's confusing is the expectation that a given Javascript object can fulfil both the function of a Mithril-specified vnode tag and any arbitrary external contracts. IMO your strawman — an esoteric stateful data object written as a class, which is also a Mithril component — is just as ambiguous a requirement as, say, a React component which is also a Mithril component, or asking why HTMLElement.onclick = ArrayInstance.push doesn't 'just work'.

Components can be written so as to fulfil the implied desirable OOP features — common methods, inheritance, persistent state reference — but they must first and foremost conform with Mithril's component & vnode specifications.

You could write an adapter, of course, but given the flexible and often brittle potential of Javascript objects, only you can judge whether it will address your particular concerns. Something like this might work:

const NumbersComponent = {
  oninit(vnode){
    this.value = new Numbers( vnode.attrs.value )
  },

  view(){
    /**/
  }
}

for( key in Numbers.prototype )
  NumbersComponent[ key ] = function(){
    Numbers.prototype[ key ].apply( this.value, arguments )
  }

While this may end up yielding satisfactory results, it certainly isn't idiomatic Mithril. Mithril (especially as of the rewrite) defines and provides the component contract for your convenience and expects you to make whatever 'business' model bindings work within that contract.

DOMVM would seem a better fit for the strawman — it goes for flexibility over hand-holding. In its own words:

It lets you - not the view layer - dictate your app architecture.

This is a stark and exceptional reversal of the norm for view libraries inasmuch as it simply provides a low-level hyperscript and virtual DOM patching API which is left to you to decide how to integrate into your existing application architecture.

[ciscoheat]

Thanks @barneycarroll, I see the Mithril component issue now, but using terms like strawman and esoteric won't really help the discussion, rather closing it early by disparaging.

The keyword is simplicity. It's not a React component or anything else. It's a simple object with identity, state and behavior that I would like to display with as little hassle as possible. Now that I know more about what's possible and not, I'll do some thinking.

[barneycarroll]

Sorry @ciscoheat, I didn't intend to disparage your argument. By 'straw man', I mean an artefact written to demonstrate a scenario (the numbers component); by 'esoteric', I mean comprising expectations that are unknowable in the receiving context — ie Numbers is esoteric in the context of Mithril. I'm trying to elucidate the nature of the unmet assumptions when trying to reconcile the behaviour of 2 constructs from different domains into a singular object that fulfils the expected behaviour of both domains.

I'm a big fan of DCI principles and don't mean to come across dismissive, but the concerns presented here aren't trivial.

Mithril tags are highly specific entities whose properties are defined and managed by Mithril — what might be considered 'esoteric' to the author of the Numbers entity.

The keyword is simplicity. It's not a React component or anything else. It's a simple object with identity, state and behavior that I would like to display with as little hassle as possible.

Mithril eschews prototypal inheritance and the other associated behaviors of Javascript 'classes' in favour of a more explicit contract. In contrast, idiomatic React usage involved extending the ReactComponent class. You could argue this is 'simpler' since it involves a common Javascript standard for defining reusable entities, given which — in the absence of any other requirements — a Numbers entity is just as simple as a ReactComponent. But even if Mithril were more 'generic' in this regard, it still comes up against the 'diamond problem' which Javascript inheritance is unable to address: how does one reconcile the intent to inherit from both Numbers and Component? The traditional abstract straw man for this problem is the task of using inheritance principles to define a Photocopier in a context where you have existing Scanner and Printer classes. It isn't straightforward using Javascript native idioms, and recent work on the language (extensions to class, static methods in place of class methods such as Object.keys) are indicative of the hidden complexity behind seemingly intuitive OOP concerns in Javascript.

FWIW you might be interested in @pygy's long standing effort into catering for divergent concerns in Mithril component definition in #1339, specifically in trying to enable components as classes and factories rather than static 'tags'.

[lhorie]

I think there isn't really a canonical most-sensible value for this in javascript. For example, back in the jQuery days, it was really common for people to get tripped over what this was in .each even though it was logically what it was supposed to be.

In backbone, we used to have to manually rebind this everywhere because the defaults were pretty much useless.

Vue allows for stuff like this out of the box:

const Counter = {
  template: `<div>{{ count }}</div>`,
  computed: {
    count () {
      return this.$store.state.count // this != Counter.computed
    }
  }
}

But I would argue that its rebinding of this is far more useful than if it did what plain Javascript would.

IMHO, this should be what's most useful. Pointing to vnode.state seems more useful to me than pointing to vnode.tag

[lhorie]

On a side note, #1484 is relevant to this discussion

[dead-claudia]

@lhorie @ciscoheat

Here's my proposed solution:

1. Have vnode.state inherit prototypically from vnode.tag (the component) instead of just shallow-cloning its own properties.
2. Use vnode.state directly instead of depending on vnode.tag.
3. Don't pass vnode.state to vnode.attrs via this, but instead, pass the vnode.attrs object itself.

This will speed up creation significantly by just setting a prototype instead of copying everything, including hooks, to vnode.state (including oninit/view/etc.), and it would simplify the renderer internals quite a bit. One perk is that vnode.attrs and vnode.state could share the exact same hook invocation logic. Another is that you wouldn't have to look up vnode.tag nearly as much, mostly just to check identity.

One caveat is that vnode.state would be invalid until the component is initialized, but mutating the state directly before creation is an anti-pattern at best.

---

For what it's worth, this bug was likely induced by @pygy changing from the prototype-aware deep cloning to just shallow cloning with own properties.

[lhorie]

vnode.state now inherits via prototype from vnode.tag