Disallow components as raw nodes

[dead-claudia]

(Branched off of this comment of mine. Also, check out #994.)

TL;DR: Components should not be valid virtual nodes.

---

The fact components are valid nodes complicates the code base from the get go. It forces a lot of unnecessary duck typing to know if a node is a component or not. Also, m.component() always creates closures, which inherently decrease performance.

Another thing is that it's not common in practice to use a raw component as a node, but instead just use m(component, arg1, arg2). Why not only use that (or the equivalent m.component)? It's fewer things to conceptually deal with.

And for mithril-objectify users, it's not possible to convert m(component, arg1, arg2) into something meaningful. Disallowing raw components as valid nodes would enable something like m(component, arg1, arg2) to be converted into something like {type: component, args: [arg1, arg2]}

This would mean making the following code invalid:

var Subcomponent = {
  view: function () {
    return m("div")
  },
}

var Component = {
  view: function () {
    return Subcomponent // Error
  },
}

I'm curious what you all think. 😄

@barneycarroll suggested creating a new branch for developing this and #994 further and see where that takes us. Also, feel free to discuss any ideas you have about this here. 😄

---

/cc @lhorie @tivac @barneycarroll @pygy @ArthurClemens @StephanHoyer @mindeavor @ciscoheat

[barneycarroll]

Thanks for this @isiahmeadows!

This is effectively reverting a change introduced in 0.2.1. The changes therein are difficult to rationalise because 0.2.1 introduced way too many changes, broke a lot of stuff, and the changelog doesn't make it clear that despite the notice that 0.2.1 was nominally abandoned in favour of 0.2.2-rc1, most of 0.2.1's changelog actually made it in under the next release. I haven't gotten around to writing up my formal proposal for how to manage process and expectations for future Mithril releases — suffice to say for now that this (breaking out discrete issues into separate PRs for debate, analysis and arbitrary validation) is a huge step in the right direction :)

I found out about the impact of this feature the hard way when @der-On notified me that my extension Exitable broke his application. I can testify that the implications for plugin developers is significant: The complexity of the Mithril run time is extra ambiguous since logic that would previously have executed during a top level view's execution is now deferred to a step that is difficult to pinpoint and reconcile. After thinking I had successfully patched Exitable to cater for the implications by modifying my m proxy, I subsequently discovered that I needed extra hooks to be able to get a handle on fully compiled virtual DOM trees.

I'm going into this level of anecdotal detail because it may prove helpful to authors of other Mithril-dependent tools trying to ensure 0.2.2 compatibility. However, I can get away with this because Exitable is a runtime executable: I suspect my fixes won't be of any use to tools like Mithril Objectify which depend upon being able to precompile virtual DOM without recourse to config proxies.

So AFAICT there currently remains a significant problem here for anyone who wants to be able to compile static virtual DOM trees from an m invocation.

---

Having said all this, I'm not blind to the huge convenience this brings to application authors. From an end user level, this brings componentised Mithril code into much easier reach. It's incredibly intuitive and terse to write component-invoking view code with the same grammar as JSX while retaining the full flexibility of custom component signatures etc. It would be a shame for end users to loose this feature because the current implementation is too complicated.

So I'd like to ask why m( component ) is necessarily so much more complicated than m( component, ...args ). Why can't we execute these component invocations recursively in one step? Surely the partial application logic isn't any more complicated — just sniff for subsequent arguments and act accordingly?

[dead-claudia]

Edit: fix incorrect word.

@barneycarroll Not quite what I was getting at. I'm not proposing removing m.component at all. I'm wanting to make m.component(Foo, arg1, arg2) not a valid node itself. Think of it as the difference between this:

// Good, still okay with this proposal
var Component = {
  view: function () {
    // Returns a wrapped vdom node
    return m.component(SubComponent)
  }
}

// What I'm proposing to make invalid
var Component = {
  view: function () {
    // Returns the literal component
    return SubComponent
  }
}

In the former case, I'm also wanting to make m.component(SubComponent) not return a component itself.

[lhorie]

My understanding of this proposal is that its goal is mainly to make life easier for mithril-objectify, and to simplify the codebase.

As I mentioned in the other issue, I don't mind the breaking change. After some dogfooding, I feel I like the m(component) syntax better than a naked component, especially since it's not as common to have a component that takes no parameters (in my code, anyways).

With that being said, I don't feel like this change would simplify the current codebase all that much, because you still need to do some type checking (currently a check for object.view, which would be replaced by a check for typeof object.tag). If I were to play devil's advocate, I could probably even argue that it's possible to add this on top of existing functionality without a breaking change.

I think the biggest impact would probably be on mithril-query, if this were to land as a breaking change. Would love to hear some thoughts from @StephanHoyer on this

For the rewrite, I'm about to start tackling components, and this proposal is actually one of the directions I was considering going because it happens to fit well with the code structure that I have in place.

Not sure if it's helpful to describe what the new vdom structure will look like but here it is: The vdom tree is now largely inspired by cito, and there are some breaking changes at the vdom structure level (which are relevant to mithril-query cc: @StephanHoyer and mithril-objectify cc: @tivac, but shouldn't affect anyone else who's just using m() calls). Here's the rundown:

• primitives are no longer wrapped in their object form, instead they follow the structure {tag: "#", children: aPrimitive}
• arrays are no longer flattened, instead they follow the structure {tag: "[", children: anArray}
• trusted html is also no longer a String object, instead it follows the structure {tag: "<", children: aString}
• a single text child is represented as a text property on its parent node, instead of the primitive structure (yes, this is an optimization, taken from Snabbdom). This won't affect mithril-objectify, as the engine is capable of handling non-optimized single text child structure, but it will affect mithril-query, since it consumes the vdom structure.

As a side effect, all node types are keyable, and all node types can have lifecycle methods (currently I have oncreate, onupdate and onremove). As I mentioned, adding components w/ a structure {tag: aComponent, attrs: {}, children: []} fits well with the structure of this new diff engine.

[StephanHoyer]

I think it's not that big of a deal since I support m(Component, args...) anyways. But I'm really not sure about this. I will dbl. check this any time soon.

[dead-claudia]

@lhorie

1. I have no attachment to the particular structure of the object.
2. You're probably right in that it can be done on top of the existing engine.

As for that proposed change, I do have a few questions/remarks:

1. Is this going to change much with the m() external API?
2. Is that {tag: "#", children: "primitive"} (and friends) just an IR? Or is that an object literal actually on the child?
3. Would it be a better idea to put the key on the vdom element itself like {tag, attrs, children, key}? At least there, it would be simpler to keep track of in the rendering, independent of attributes.
4. I really like the {tag: component, attrs: {}, children: []} idea for components.
5. I really like the extreme consistency in representation. That opens the door for a lot of optimizations. That thing is going to fly (well...as long as it has all the necessary tests 😄).

[lhorie]

1. the plan is to keep the hyperscript API the same, e.g. m(".foo", "bar") will spit out {tag: "div", attrs: {class: "foo"}, text: "bar"} so you don't need to fudge around on the view code to return a performant vdom structure
2. not sure what you mean. Basically, m.trust("foo") will spit out {tag: "<", children: "foo"} and m("div", "a", "b") will spit out {tag: "div", children: [{tag: "#", children: "a"}, {tag: "#", children: "b"}]}
3. tried toying w/ it, but perf suffered. Inferno spike manages to get away with using a delete somehow, so there might be a way around it that I'm not aware of.
4. me too, it's a stolen idea :)
5. I'm clocking about 221 mocha tests for the diff engine right now. I have just short of 700 assertions for the rest of the system. Even mocks are tested now...

[dead-claudia]

@lhorie

• You answered my second question well enough. 😄 (I phrased that poorly. My mistake. You basically answered that the second case, as in it's an object literal on the children array.)
• You could do a hasOwnProperty check for the key, and only delete the key or shallow-copy the object omitting the key if it exists. Otherwise, it's a pass-through.
• Totally stolen from Cito React Mercury virtual-dom... I give up. 😆
• Yay for tests! At least I can be a little more sure I'm not as likely to screw things up again. 😄

[dead-claudia]

And the other reason I suggested this was because of memory. POJOs take up a lot less space than a dual closure with two bound functions.

[barneycarroll]

This doesn't affect any of my code in practice, so I'm easy. Removing pointless intermediary representations sounds good though.

[tivac]

I didn't even realize that you could return naked components, so I don't use it and am thus fine w/ removing it. Were the docs ever updated to reference that support?

I don't necessarily see how it makes life easier for mithril-objectify, because determining if the first argument to m() is a string or object is complicated so I haven't done any work on that front.

@lhorie thank you SO MUCH for the details about [email protected], that's very exciting to hear about the progress being made. The unified VDOM structure seems totally reasonable, and updating mithril-objectify to support it shouldn't take me too long once there's actual code in the wild.

[dead-claudia]

@tivac You can at least do it for m.component, but I doubt that's going to mean much. You do have a point.

[dead-claudia]

I almost have a patch ready. It hasn't had the perf tuning yet, but I'm planning on putting it up in a few, anyways, to solicit feedback.

Basically, it converts nodes to use the following structure:

interface VirtualNode {
  tag: Component;
  args: any[];
  attrs: {keys: null | string | number};
}

Example:

assert.deepEqual(
  m(Component, {key: 1}, foo, bar, baz),
  {
    tag: Component,
    args: [{key: 1}, foo, bar, baz],
    attrs: {key: 1}
  }
)

assert.deepEqual(
  m(Component, foo, bar, baz),
  {
    tag: Component,
    args: [foo, bar, baz],
    attrs: {key: null}
  }
)

[lhorie]

Is this still active? Is there anyone interested in it?

[barneycarroll]

@lhorie AFAIC this is comprehensively resolved by the rewrite's virtual node API. Because this issue doesn't deal with any low level practical issues in v0.X (it's about hacking and/or high level developer concerns, not bugs), anybody with a strong stake in this should be prepared to upgrade and debate #1086. Vote to close.

[lhorie]

Alright, closing