vnode.dom in component with array at the top level returns the first child

[CreaturesInUnitards]

Expected: vnode.dom would return a NodeList or an Array

Actual:

var component = {
     oncreate: function(vnode){
          console.log(vnode.dom)
     },
     view: function(){
          return [
               m('.first'),
               m('.second')
          ]
     }
}

This example logs out <div class="first"></div>

[spacejack]

I believe that's intended behaviour - dom is the first element. You can get the next element with dom.nextElementSibling

[CreaturesInUnitards]

It seems too prominent to not have been considered; I'd just like to register the counter-argument that in every other case, vnode.dom is technically the first element, yes, but in feel it's the component's outermost container. This behavior is surprising and counter-intuitive, IMHO.

Another side effect is that this gives us no simple dom-level analogue for the container itself, meaning the array being returned by our view. The most natural, unsurprising behavior would be for vnode.dom to return an array or nodelist, otherwise we're looking at vnode.dom.parentNode.childNodes.

Which is just silly.

[CreaturesInUnitards]

To be clear: the only reason this is at all significant in my mind is that we're talking about vnode.dom; we're already accessing the rendered HTML.

In that case it would be perfectly reasonable to want to apply the same bit of code to all the elements in the container, which would expectedly be the same, since the author has specified a containing array.

[cavemansspa]

i thought m.fragment https://github.com/lhorie/mithril.js/blob/next/docs/fragment.md addresses this?

[CreaturesInUnitards]

@cavemansspa No, AFAICT it's the same behavior:

var component = {
     view: function(){
          return m.fragment(
               { oncreate: function(vnode){console.log(vnode.dom)} }
               ,[
                    m('.first'),
                    m('.second')
               ]
          )
     }
}

This still logs out the first div.

[dead-claudia]

Here's a couple questions:

1. What should vnode.dom be when no nodes are rendered? (Most likely undefined.)
2. What should vnode.dom be when 2 or more nodes are rendered?

The second question is more important here, and there are multiple options:

1. An array of nodes
2. A DocumentFragment or NodeList
3. The first node

[CreaturesInUnitards]

What should vnode.dom be when no nodes are rendered? (Most likely undefined.)

I would assume undefined, but I can see use cases for an explicit null, e.g. idiomatic Mithril relies heavily on ternaries for conditionally rendering nodes, and often takes the form
SomeCondition ? m(MyComponent) : null
...so maybe some might find that distinction useful. The checks are trivial either way, so it matters less.

What should vnode.dom be when 2 or more nodes are rendered?

As I said above, I think it has to be an array or NodeList — but preferrably an array — of the actual DOM nodes. A DocumentFragment is an interesting idea, but it's indirect; its children would be copies of the actually rendered nodes in question. The current behavior — returning the first node — is surprising and somewhat confusing.

HTML, being an explicitly hierarchical medium, encourages and enforces matryoshka-driven thinking. And m(), in practice, does nothing to contradict that. So here we have a view, whose author has chosen to forego explicit hyperscript as a component's outermost container, and instead returns an array. So for comparison:

{ view: function(vnode){ 
   return m('') 
} }

Here we ask our view to return a vnode. In this case vnode.dom refers to a directly addressable representation of that vnode.

{ view: function(vnode){ 
   return m(''
      , m(''
         , m('')
      )
   ) 
} }

Same type of request, same type of return value. The nuance here is that the reasonable author who isn't diving into Mithril's implementation details doesn't see vnode.dom merely returning a single node; they see it returning the outermost container, in the form they explicitly specified.

{ view: function(vnode){ 
   return [ m(''), m('') ] 
} }

Here, we ask our view to return an array of vnodes. It follows intuitively that in this case vnode.dom could be expected to refer to, as above, the outermost container in the form they explicitly specified: an array of directly addressable representations of those vnodes.

Certainly, the argument can be made that vnode.dom is expected to return an actual piece of the DOM, which IMHO makes the argument for returning a NodeList. In practice, though, working with a NodeList sucks in comparison, and authors too-often end up writing things like

[].map.call(myNodeList, function(n){...})

I suppose if this were implemented, I'd say we return an array unless returning a NodeList has better perf.

[thysultan]

Implementation wise a NodeList would be hard/border-line impossible since vnode.dom.parentNode.childNodes is not necessarily equal to the childNodes returned from render i.e fragments can exist within a list; Kinda wish there was aDocumentFragment equivalent with persistence that acted like an Element Node added to the DOM spec.

[leeoniya]

you could make a .dom es5 getter that creates a nodelist on demand by iterating a from/to index. not sure how feasable it is in mithril's architecture or perf implications.

[dead-claudia]

Here's my proposal for how it could be done reasonably:

1. If a non-array, non-fragment is returned, and inside of vnode attrs hooks themselves, let vnode.dom be the backing node of the raw vnode, if any.
   • When a component vnode is returned, it's just undefined.
   • When a DOM vnode is returned, it's the normal vnode.dom known now.
   • When a non-vnode is returned, it's the resulting text node.
   • In attrs hooks, it's either undefined or the normal vnode.dom, depending on the backing vnode's type.
2. If an array or fragment is returned, it's an array of the above.
3. vnode.dom is to remain undefined before oncreate, except in DOM vnodes when recycling.

[dead-claudia]

@leeoniya vnode.dom as a getter would be highly impractical because Mithril uses that property extensively internally, so not only would it slow things down, it'd also require broad changes to the renderer.

My proposal avoids this by ensuring vnode.dom can only be an array within components, where it's only written to. So that would limit what changes would be required.

[CreaturesInUnitards]

@isiahmeadows LGTM, with the single stipulation (from where I sit) that if this negatively impacts perf in the non-array scenarios I'd say ditch it, call it academic, and Let Us Never Speak of This Again.

[pygy]

@CreaturesInUnitards You can achieve what you're after using the vnode.domSize property, which is undefined for all nodes but fragments and trusted strings.

https://mithril.js.org/vnodes.html#structure

This helper will collect the vnodes into an array.

function collectFragment(vnode) {
  var res = []
  var i = 0
  var dom = vnode.dom
  if (vnode.domSize !== 0) do {
    res.push(dom)
    dom = dom.nextElementSibling
  } while (++i < vnode.domSize)
  return res
}

Live here: http://jsbin.com/hiranudeyo/1/edit?js,console,output

[CreaturesInUnitards]

@pygy thanks! But I'm actually not trying to achieve this at all. In fact I can't imagine ever wanting to do this myself, but the fact remains that Mithril's behavior is surprising; I just raised a flag. 😄

[pygy]

@CreaturesInUnitards then I misunderstood this:

In that case it would be perfectly reasonable to want to apply the same bit of code to all the elements in the container, which would expectedly be the same, since the author has specified a containing array.

What did you mean?

Edit: Reading the vnode docs, the vnode.dom and vnode.domSize descriptions don't mention components at all, that should be improved.

@isiahmeadows I'd leave it as is, personally. Extracting the list from vnode.dom and vnode.domSize is trivial in user code if ever needed.

[spacejack]

Well a change to what vnode.dom is would be breaking behaviour for anyone using dom.nextElementSibling (like I am...)

[CreaturesInUnitards]

@pygy Yeah, that's awkwardly written, isn't it?

I mean that if my code is this:

[m('.foo'), m('.bar')]

I could reasonably expect vnode.dom to return this:

[
   <div class="foo"></div>,
   <div class="bar"></div>
]

[pygy]

That would entail either supplemental allocations or branches in the core, and it can be achieved in user land... It's not an oversight, vnode.domSize is there to cover that need. It may not be the most user friendly API, admittedly, but it is efficient.

I edited the snippet above to handle zero-sized fragments.

[CreaturesInUnitards]

@spacejack Clearly, and I'm certainly not going to jump up & down if this change isn't made.

The only skin I have in this game is that I want to keep improving the framework, specifically its reach and conceptual accessibility. In that context, surprising/arbitrary behavior is a real bummer. I think returning the first element is both surprising and arbitrary, although not totally unreasonable. I want this change only if it's easy and performant.

@pygy wrt my above paragraph and your most recent comment, I thank you for your insight and will now close this issue.

[ww9]

I just experienced the confusion reported by @CreaturesInUnitards. I expected vnode.dom to contain 3 elements but it only had the first. After 10 minutes debugging I found this GitHub issue. Here's my input:

• Yes, vnode.dom having just the first element is unintuitive

• After reading this Issue and pondering, I understand it only returns the first element for performance reasons and I'm fine with it

• Perhaps calling it something along the lines of vnode.firstDom or vnode.firstElement would improve users expectations and avoid confusion

[dead-claudia]

@ww9 Please file a new issue with that suggestion. 👍

BTW, you can detect the number of elements via vnode.domSize. You can grab an array containing all the linked elements of any vnode via this:

function getElems(vnode) {
    var ret = [], dom = vnode.dom
    for (var i = 0; i < vnode.domSize; i++) {
        ret.push(dom)
        dom = dom.nextSibling
    }
    return ret
}