createNode/h attaches key to vnode.

[rbiggs]

I noticed that createNode/h function has been updated to attach the key directly to the virtual node:

{
  nodeName: name,
  attributes: attributes || {},
  children: children,
  key: attributes && attributes.key
}

Here, you assign an attribute key directly to the virtual node. Yet whenever you pass an attribute name to updateAttribute, that is always gotten from the virtual node's attributes property, not from the virtual node's key property assigned above. So I'm wondering why bother with this assignment when it is never used for diffing or patching. Preact does something similar, but it checks for a key first on the virtual node and then on the virtual node's props object.

[jorgebucaran]

@rbiggs Yet whenever you pass an attribute name to updateAttribute, that is always gotten from the virtual node's attributes property, not from the virtual node's key property assigned above.

I am not sure what you mean? Do you mean to simplify updateAttribute's if (name === "key") { check?

is never used for diffing or patching...

We use in getKey.

function getKey(node) {
  return node ? node.key : null
}

[rbiggs]

Oh, doh. didn't notice that one.

[jorgebucaran]

@rbiggs What about the other one? If there's room for improvements I'm all 👂s.

[rbiggs]

Not sure what the other one you're referring to. Mind elaborating?

[jorgebucaran]

Hmm, I think this part:

Yet whenever you pass an attribute name to updateAttribute, that is always gotten from the virtual node's attributes property, not from the virtual node's key property assigned above.

[rbiggs]

Keys to the Kingdom of Virtual Dom

This is what's bouncing around in my head about how virtual doms are handling keys. We're talking React, Inferno, Preact and Picodom/Ultradom. First of all, don't take anything I say personally. This is just my opinion or pet peeve and does not even affect why or how I choose a library to use.

First let's go back in time. When Picodom was at version 1.0.0 the h function attached all JSX element props to the virtual nodes's props attribute. This meant that a key prop would also be assigned to the virtual node's props attribute.

The assignment was like this:

{ 
  type: type, 
  props: props || {}, 
  children: children 
}

If I had some JSX that used keys:

function List({data}) {
  return (
    <ul class='list'>
      {
        data.map(item => <li key={item.key} class='list-item'>{item.name}</li>)
      }
    </ul>
  )
}

I would get a virtual node like this:

{
  children: [
    {
      children: ['Item One'],
      props: {
        class: 'list-item',
        key: 'a101'
      },
      type: 'li'
    },
    {
      children: ['Item Two'],
      props: {
        class: 'list-item',
        key: 'a102'
      },
      type: 'li'
    }
  ],
  props: { class: 'list' },
  type: 'ul'
}

This makes since because the virtual node matches the JSX.

To get a key from a virtual node Picodom had:

function getKey(node) {
  if (node && node.props) {
    return node.props.key
  }
}

Changes to Key Assignment

At version 2.0.0, Picodom was changed so that the h, function assigned the key property directly to the virtual node:

{
  nodeName: name,
  attributes: attributes || {},
  children: children,
  key: attributes && attributes.key
}

Interestingly, because a key is a JavaScript primitive type (string or number), the assignment is by value, not by reference. On top of that, this assignment results in the key value existing in two different places for the node. If we look at what our previous JSX code produces with this new key assignment, it looks like this:

{
  children: [
    {
      children: ['Item Two'],
      key: 'a101',
      props: {
        class: 'list-item',
        key: 'a101'
      },
      type: 'li'
    },
    {
      children: ['Item Two']
      key: 'a102',
      props: {
        class: 'list-item',
        key: 'a102'
      },
      type: 'li',
    }
  ],
  props: { class: 'list' },
  type: 'ul'
}

As you can see, each virtual node that has a key, has it in two places. Since the assignment is by value, it is possible that in some really weird edge case the two values could diverge. Of course, all access of keys in Picodom 2.0.0 and later are done through the getKey function, which only accesses the key from node.key. So what happens to node.props.key doesn't matter. Still, that there could be a divergence and that the value now exists in two places feels like this is a leaky abstraction.

I'm assuming this change in key management was made to match Preact, since the names were also changed to match Preact's usage. Inferno and React use a different naming scheme for their virtual nodes: type and props. No biggie. They're just names used internally.

Inferno and React do not have the problem that Ultradom and Preact have with keys existing in two places on a virtual node. Both Inferno and React remove the key from the props object. Actually React does not entirely remove the key from props. It just sets its value to undefined. Still, the result is only one property per virtual node with a key value.

Here's the Inferno object. Intersting to note that they decided to also move className out of props and up onto the virtual node:

{
  children: [
    {
      children: ['Item One'],
      className: 'list-item',
      key: 'a101',
      props: {},
      type: 'li'
    },
    {
      children: ['Item Two'],
      className: 'list-item',
      key: 'a102',
      props: {},
      type: 'li'
    }
  ],
  className: 'list',
  props: {},
  type: 'ul'
}

If we look at the virtual node created by React, minus some React-specific properties, it would look like this (Notice that React moves children into the props attribute):

{
  children: [
    {
      key: 'a101',
      props: {
        children: ['Item One'],
        className: 'list-item',
        key: undefined
      },
      type: 'li'
    },
    {
      key: 'a102',
      props: {
        children: ['Item Two'],
        className: 'list-item',
        key: undefined
      },
      type: 'li'
    }
  ],
  props: { className: 'list' },
  type: 'ul'
}

As you can see, Inferno and React do some cleanup with the keys after moving them. Everyone seems to think they need to move keys from props directly onto the the virtual node, This leads me to wonder why. I've never seen an explanation anywhere as to why such a move would be good.

Premature Optimization?

I imagine some developer at Facebook years ago though that it would be faster to access a key if it was directly on the virtual node, node.key instead of node.props.key. That's got to be a performance gain of 0.025 millisecond. However, once this pattern was established, it has continued unchanged to this day. Then when Dominic Gannaway and Jason Miller created Inferno and Preact, they looked at that pattern and copied it.

If this is for speed improvement, then why not move all properties out of node.props and directly onto the virtual node? Incidentally, Inferno does move className from props to the virtual node, but not any other props, and then React moves children down onto props. Can library authors please be consistent with this?

What bothers me about this pattern is that as a developer I assign the properties directly on the node, yet when I examine the virtual node, some of them may somtime have been moved depending on the library. This trend to move properties to new locations on a virtual node feels very whimsical to me. Why not just leave them as props? All this moving around means that the virtual nodes don't match the JSX markup. The whole point of JSX, props and passing state down is that it's supposed to make it easy to reason about. I don't see this is the case when the virtual nodes don't match the JSX that produces them.

That's it. Sorry for the long rant. Maybe I'm getting OCD in my old age.

Cleaning Up h in Ultradom

If you intend on sticking with the current choice to add a key directly to the virtual node, you could clean up the virtual node. For h, this could be as simple as:

var key
if (attributes && attributes.key) {
  // assign by value
  key = attributes.key
  // remove duplicate from attributes
  delete attributes.key
}

return typeof name === "function"
  ? name(attributes || {}, children) // h(Component)
  : {
      nodeName: name,
      attributes: attributes || {},
      children: children,
      key: key
    }

The above would create a virtual node without key duplication:

{
  attributes: {
    class: 'list'
  },
  children: [
    {
      attributes: {
        class: 'list-item'
      },
      children: ['Item One'],
      key: 'a101',
      nodeName: 'li'
    },
    {
      attributes: {
        class: 'list-item'
      },
      children: ['Item Two'],
      key: 'a102',
      nodeName: 'li'
    }
  ],
  key: undefined,
  nodeName: 'ul'
}

I guess I must be the only person in the world that cares about my vnodes. ☹️ Any way, that's my point from the previous comment about:

Yet whenever you pass an attribute name to updateAttribute, that is always gotten from the virtual node's attributes property, not from the virtual node's key property assigned above.

Getting rid of an unused key value to aleviate this issue.

[trueadm]

@rbiggs I just saw your comment, so I thought I'd reply.

If this is for speed improvement, then why not move all properties out of node.props and directly onto the virtual node? Incidentally, Inferno does move className from props to the virtual node, but not any other props, and then React moves children down onto props.

Having all the props on the VNode would indeed be the fastest in most cases and result in less memory. The issue is that you'd break the hidden class of the VNode by doing so. When working on Inferno, the team and I found that className was the most commonly used prop when rendering to the DOM on almost all projects we looked over. For large lists of nodes, where frequent access to key is made, by having the VNode remain monomorphic, you can much better performance than if you were to access the property on props (which would likely be polymorphic in most cases).