Make boolean "false" a valid attribute value

[frenzzy]

before: <div data-attr={false} /> => <div></div>
after: <div data-attr={false} /> => <div data-attr="false"></div>

Useful for attributes such as draggable and for custom data-attributes.

The does not affect props like checked and they will continue work as previously.
Hope this change also decrease library size on a few bytes :)

[codecov]

Codecov Report

Merging #592 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #592   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         136    136           
  Branches       41     41           
=====================================
  Hits          136    136

Impacted Files	Coverage Δ
src/index.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25b85ef...4b78feb. Read the comment docs.

[jorgebucaran]

I need to do a bit more testing to make sure this does not break other stuff.

Good job @frenzzy!

[frenzzy]

Oh.. looks like this PR introduces inconsistency between client and server side rendering because on server side we can't check if attribute belongs to element without whitelisting. It is much easier to just not render falsey attributes. See react-dom/src/shared/DOMProperty.js

[SkaterDad]

I kind of like the current behavior. For some attributes, like disabled, it's preferable (to me) to remove it completely when false. Or would it still function as expected if setting to false?

For now, if you want to set an attribute to false, do strings work?

EDIT: Some quick testing leads me to believe that for <input disabled />, you need the attribute to completely disappear to re-enable it. We could use null for that, but it's more intuitive to model enable/disabled state with a boolean, I think.

    // renders a disabled <input> to the DOM
    h('input', { oncreate: (el) => { el.setAttribute('disabled', false)} })

[frenzzy]

After this PR only non-standard attributes will render false as a string:

• <div data-custom={false} /> => <div data-custom="false"></div>
• <button disabled={false} /> => <button></button>

[jorgebucaran]

@SkaterDad I have a feeling this has been discussed before and we agreed with each other.

For some attributes, like disabled, it's preferable (to me) to remove it completely when false...

Why was that again? 🤔

[jorgebucaran]

@frenzzy This needs more tests and also more cowbell. 🐮🔔

[SkaterDad]

@frenzzy After this PR only non-standard attributes will render false as a string:

Can you put tests to verify this?

When I code this:

    h('input', { oncreate: (el) => { el.setAttribute('disabled', false)} })

Chrome renders this, which is still a disabled element functionally:

<input disabled="false">

[frenzzy]

Yes, I'm scared about this change too. But do not have a real app to test this yet.

[frenzzy]

@SkaterDad there are check in code:

const element = <input>
const attributeName = 'disabled'
const value = false

if (attributeName in element) {
  element[attributeName] = value
  // => <input />
} else {
  // use element.setAttribute
}

https://github.com/hyperapp/hyperapp/blob/c1a22ae45817f681a43059db27730d82d2704632/src/index.js#L140-L141

[SkaterDad]

@frenzzy Thanks for pointing that out. I must have missed that hyperapp distinguished between props and attributes again! 👍

My use cases shouldn't be affected by this PR, then, since I'm not doing any SSR currently.

[frenzzy]

I gonna close this PR because it introduces mismatching between client and server side rendering and does not help in any case to render boolean false as a string to reserved DOM attributes like draggable, you need to pass "false" as a string anyway.