class={undefined} becomes class="undefined"

[edemaine]

Describe the bug

<div class={undefined}/> renders as <div class="undefined"></div>. Or more precisely, it compiles to _el$.className = undefined; which is equivalent to _el$.className = "undefined"; apparently.

By contrast, <div foo={undefined}/> renders as <div/>. Or more precisely, it compiles to setAttribute(_el$, "foo", undefined); which is equivalent to removing the attribute.

Your Example Website or App

https://playground.solidjs.com/?hash=854479419&version=1.3.13

Steps to Reproduce the Bug or Issue

1. Go to https://playground.solidjs.com/?hash=854479419&version=1.3.13
2. Inspect the button element
3. Observe attributes

Expected behavior

I expect <div class={undefined}/> to behave like <div foo={undefined}/> in that both omit the attribute.

Screenshots or Videos

No response

Platform

• OS: [e.g. macOS, Windows, Linux]
• Browser: [e.g. Chrome, Safari, Firefox]
• Version: [e.g. 91.1]

Additional context

I guess the main alternative here is to compile to using setAttribute instead of className. I'm not sure whether there are unintended consequences to that (bigger code output, maybe efficiency?).

Pointed out by RodrigOv on Discord in #general.

[ryansolid]

Yep legit and really quite annoying. To have a property follow attribute rules. I figured properties weren't subject to this, but this one is. className is more performant and I chose to call it out because it's a common one to change rapidly. Looks like it needs its own handling.

[edemaine]

OK, I was worried that className was more efficient. A simple fix would be to compile class={x} to _el$.className = x ?? '' or _el$.className = x || ''. Though I believe this has slightly different semantics — it creates a class attribute with an empty value, as opposed to removing the attribute. Maybe that's fine though. (Certainly better than currently.)

[ryansolid]

Yeah I was leaning that way. Also have to consider special case in spreads. This is how Inferno handles it though.. It does remove it as an attribute but sets it as a property.

https://github.com/infernojs/inferno/blob/master/packages/inferno/src/DOM/patching.ts#L227-L235

[edemaine]

That does look like a good way to do it. removeProperty seems like the only way to properly implement class={undefined}, but otherwise className should work fine. Let me know if you want a PR.

[ryansolid]

Yeah still requires touching a bunch of stuff including compiled output.. and adding a className helper. This is probably a minor version sort of change.