Mithril.js: Falsy values and booleans as class attributes are all over the place

[pygy]

http://jsbin.com/yeyesucixe/1/edit?html,js,console

The way null, invalid, true and false are handled as class/className attributes is highly inconsistent depending on whether the are set using raw vnodes, hyperscript (with or without classes in the selector), and depending on the element type (elements within a namespace like svg are treated differently).

The bin above only demoes what happens on element creation, after neutering the recycling pool. What happens on update and when nodes are recycled is even more funky.

Edit: here are the attributes generated by hyperscript(): http://jsbin.com/roqajexobu/1/edit?html,js,console

The fact that we have both class and className defined in some cases may be problematic for 1) perf and 2) for custom elements where side effects may occur when class and/or className are set.

See also: #1769 #1764

[pygy]

@lhorie do you remember why you chose to normalize classes that come from the selector to className rather than class?

[lhorie]

because setting el.className is faster than el.setAttribute("class", ...)

[leeoniya]

i special-case "id" and "class" in my attrs loops to set them as props. works well.

[pygy]

So we could default to vnode.attrs.class (unless className is present in user-provided attrs), and then use a the property setter in setAttr() no matter what. That way m() wouldn't produce attrs with both class and className.

• What should render do when both class and className are specifed? => I'd say "undefined behavior"
• What happens with when falsy values (or true) are used in hyperscript/raw attrs?
  
  
  
  • What happens in presence of classes that come from the selector?
  
  
  • How does it mix with elements that have a namespace?
  
  
  • currently the setAttrs() path is taken
  
  
  • How does it mix with custom elements ()?
  
  
  • currently the setAttrs() path is taken
  
  
  • How does it mix with the way we usually handle attributes with boolean values?
  
  

[lhorie]

What should render do when both class and className are specifed? => I'd say "undefined behavior"

I'd say class should take the value of whichever is last when iterated over (so basically, undefined behavior)

What happens with when falsy values (or true) are used in hyperscript/raw attrs?

This depends on what attribute/property we are talking about. {readonly: false} means there's no attribute. {title: false} means a string "false". Generally, I think it should follow this rule: Does a property exist with the specified name? If yes, then el[attr] = value and whatever the setter does is the expected behavior. If no, cast to string (because that's what setAttribute does)

[pygy]

So this is is a bug then (elements with a namespace skip the block that ends up setting properties):

test({tag:'svg', attrs: {class: null}})
test({tag:'svg', attrs: {class: undefined}})
test({tag:'svg', attrs: {class: false}})
test({tag:'svg', attrs: {class: true}})
test({tag:'svg', attrs: {class: ""}})

<svg class="null"></svg>
<svg></svg>
<svg></svg>
<svg class=""></svg>
<svg class=""></svg>

test({tag:'svg', attrs: {className: null}})
test({tag:'svg', attrs: {className: undefined}})
test({tag:'svg', attrs: {className: false}})
test({tag:'svg', attrs: {className: true}})
test({tag:'svg', attrs: {className: ""}})

<svg class="null"></svg>
<svg></svg>
<svg></svg>
<svg className=""></svg>
<svg class=""></svg>

Do you have something in mind for undefined and null?

[isiahmeadows]

In a vdom experiment I'm working on, I compute the selector and normalize it while rendering instead of while creating the node, applying it like a second set of attributes. It avoids the whole mess of what to normalize things to quite nicely, since it's purely an implementation detail how it's handled rather than an API issue on how class vs className are handled. It's also simpler to implement due to not needing to do a non-trivial merge, even though it's inelegantly more coupled to the rendering (selector reconciliation sounds like a construction concern, not a rendering concern).

We'd still need to expose an explicit API for reconciling the selectors and attributes in-place, for testing purposes (asserting two trees match is useful) and for mithril-node-render, etc. It doesn't need bundled with the main implementation, but it's a necessity for a few key use cases.

---

To explain what I mean in simplified pseudocode:

// Current
function m(tag, attrs, ...children) {
    attrs = cloneAttributes(vnode, attrs)
    mergeAttrs(attrs, compileSelector(tag))
    return create(tag, attrs, children)
}

function renderAttrs(vnode) {
    applyAttrs(vnode.dom, vnode.attrs)
}

// Proposed
function m(tag, attrs, ...children) {
    return create(tag, attrs, children)
}

function renderAttrs(vnode) {
    var cached = compileSelector(vnode.tag)
    applyAttrs(vnode.dom, cached)
    applyAttrs(vnode.dom, vnode.attrs)
    vnode.dom.className = cached.className + vnode.attrs.className
}

[isiahmeadows]

Oh, and the above would be a major breaking change.

[pygy]

@isiahmeadows This is puzzling...

How do you check element identity? currently we replace the component/DOM element if vnode.tag !== old.tag... Do you keep a cache as well for the element, or replace the nodes when the attributes differ?

Also what are the benefits to move this to render time? What information do you have at that point that you don't already have when building the tree? Do you somehow take advantage of the old values?

[isiahmeadows]

@pygy My comparison becomes a little more indirect:

function hasDiff(oldTag, newTag, out) {
  if (typeof oldTag === "string") {
    out.old = selectorCache[oldTag]
    if (typeof newTag !== "string") return false
    out.new = compileSelector(newTag)
    return out.old.tag === out.new.tag
  } else {
    if (typeof newTag !== "string") return oldTag === newTag
    out.new = compileSelector(newTag)
    return false
  }
}

My model is slightly different, because I won't have native component support. (It's conceptually going to be closer to Elm than React, just a bit more stateful.)

[pygy]

In #1867, @porsager suggested:

Currently mithril throws if a false value is passed to the style attribute since it directly tries to set it on the dom.style property which isn't allowed.

The reason I'd like to see falsy values converted to an empty object are the same reasons as for why false values in vnodes are converted to empty vnodes.

Currently I do the following:

m('div', {
 style: green ? { background: 'green' } : {}
})

Whereas it to me would seem more idiomatic mithril to be able to do:

m('div', {
 style: green && { background: 'green' }
})

What do you think?

To which I responded"

Agreed, and more generally, false attributes/properties may be consired equivalent to a lack of attribute (unless some properties have different behaviour when set to either null or false).

1865 is already refactoring the attributes code.

and @isiahmeadows chimed in as well:

Note that this general thing was almost discussed to death in #1014, and here's a quick summary of the two main things considered there, including both the good and bad:

1. Checking true/false is generally safe and would provide a nice, short sugar. It's also fairly trivial to implement, and is non-breaking enough to land in a minor version increment. But some people would quickly start doing things like this and find themselves running into problems:

   // This would return `0` when `this.times === 0`
   this.times && m("button", {onclick: () => this.reset()}, "Reset"),
   
   // It would need corrected to one of these
   !this.times || m("button", {onclick: () => this.reset()}, "Reset"),
   this.times !== 0 && m("button", {onclick: () => this.reset()}, "Reset"),
   
   // The current equivalent
   this.times ? m("button", {onclick: () => this.reset()}, "Reset") : null,
   

For this particular one, I'm +0 on it, and most others in that issue were weakly supportive of it. It would be nice to have, but I have concerns on how often people will run into this particular gotcha (it's at least easily debuggable).

[snip: another proposal that would treat all falsy values as absent]

Another thing to take into account is the non-reflected attributes that take literal "true" and "false" values, like aria-invalid. Not sure why the invented yet another boolean representation, but they did. For those, having true converted to "true" would be quite useful.

Here's what happens currently: http://jsbin.com/cejiwozosu/1/edit?js,console

[isiahmeadows]

@pygy Regarding my response, it might as well just be ignored. I misread the issue, which is easy to do in a rush. 😞