New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Falsy values and booleans as class attributes are all over the place #1773

Closed
pygy opened this Issue Apr 4, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@pygy
Copy link
Member

pygy commented Apr 4, 2017

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 pygy added the bug label Apr 4, 2017

@pygy pygy added this to the 1.2.0 milestone Apr 4, 2017

@pygy pygy changed the title Class attributes are all over the place Falsy values and booleans in class attributes are all over the place Apr 4, 2017

@pygy pygy changed the title Falsy values and booleans in class attributes are all over the place Falsy values and booleans as class attributes are all over the place Apr 4, 2017

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

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

@lhorie

This comment has been minimized.

Copy link
Member

lhorie commented Apr 4, 2017

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

@leeoniya

This comment has been minimized.

Copy link
Contributor

leeoniya commented Apr 4, 2017

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

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Member

lhorie commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 4, 2017

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

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 4, 2017

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

@pygy

This comment has been minimized.

Copy link
Member

pygy commented Apr 4, 2017

@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

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Apr 5, 2017

@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

This comment has been minimized.

Copy link
Member

pygy commented Jun 3, 2017

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

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Jun 4, 2017

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

@pygy pygy modified the milestones: 1.2.0, 2.0.0 Jun 1, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Jun 2, 2018

pygy added a commit to pygy/mithril.js that referenced this issue Jun 2, 2018

@pygy pygy referenced this issue Jun 2, 2018

Merged

Hyperscript: fix #1773, fix #2172 #2174

9 of 11 tasks complete

@pygy pygy closed this in 92b22fe Jun 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment