Hyperapp: Updating children props dynamically ignores "0"

[rajaraodv]

I have a Parent component, that takes children components. I want to dynamically add props to each children so I’m adding “index” in a loop. Every child element is supposed to have attributes "index=0", "index=1" and so on. But the generated DOM's 1st element doesn’t have the attribute "index=0" while the rest of them have "index=1", "index=2" and so on.

Below codepen reproduces the issue:
https://codepen.io/rajaraodv/pen/aLPWgd

<div>
  <h1>raja</h1>    <-- notice that the 1st element is missing index="0"
  <h2 index="1"> rao</h2>
</div>

[jorgebucaran]

A solution could be in setElementProp, instead of evaluating value, that fails is value is falsy, we could test value != null:

if (typeof value !== "function") {
  if (value != null) { 
    element.setAttribute(name, value)
  } else {
    element.removeAttribute(name)
  }
}

So, falsy values will not result in calling removeAttribute, but is it correct? Don't we want to call removeAttribute for falsy values? 🤔

/cc @rajaraodv @zaceno @okwolf

[lazarljubenovic]

I'd remove them explicitly for undefined (===), not even null.

null, 0 and false are falsy but are values explicitly assigned by a programmer. undefined means that something either wasn't touched by a developer or was explicitly changed back to "nothing" -- which is the behavior we're looking for here.

[jorgebucaran]

Thanks @lazarljubenovic. While I wouldn't encourage != null if you I can avoid it (and one can totally avoid it in 99.99% of situations), in this case is fine because we are (in addition to want to save bytes 😉) giving you the ability to set any property as null as a way to tell Hyperapp to remove it. This also fixes @rajaraodv's bug.

---

Come to think about I am only worried about empty strings. Does it make sense to remove attributes when set to an empty string? (current behavior) or should we simply: setAttribute(name, value) even if value is an empty string?

[infinnie]

@JorgeBucaran checked, selected, etc.

[jorgebucaran]

@infinnie What do you mean?

[infinnie]

@JorgeBucaran I mean attributes like checked, selected, etc would never get removed if only set to "".

[jorgebucaran]

@infinnie Ah, yes, you are right.

[jorgebucaran]

@infinnie @lazarljubenovic @rajaraodv So, we want to remove attributes for falsies, except "". Does that sound good?

[rajaraodv]

Why remove any attribute at all unless undefined?

[zaceno]

@rajaraodv for example:

h('button', {disabled: true}, [...])

should render into

<button disabled>...</button>

So what should

h('button', {disabled: false}, [...])

render into, if not

<button>...</button>

?

Same for checked & selected and probably a couple more I'm forgetting.

EDIT: Use code blocks.

[rajaraodv]

So what should h('button', {disabled: false}, [...]) render into?

It should render into: <button disabled="false">test</button>. Because disabled=false is treated by HTML as disabled. If you want to enable it, then you shouldn't send that attribute at all to the VDOM.

[andyrj]

I agree with @rajaraodv it seems to make the most sense to only remove if the attribute is undefined, otherwise you have those weird cases if you happen to want an attribute set to 0/false/""/null.

[SkaterDad]

I vote for removing attributes that are null or undefined. I think those are the most intuitive values to set for removing an attribute.

[zaceno]

@rajaraodv Then, by the same token,

h('button', {disabled: true}, [...])

should become

<button disabled="true">...</button>

right?

What should we write in order to achieve:

<button disabled>...</button>

(valueless attribute)?

[zaceno]

@rajarodv

Because disabled=false is treated by HTML as disabled

... is this true in all browsers? What about <option selected="foo">bar</option> -- is that treated as selected?

[zaceno]

So if I understand the suggestion from @rajaraodv, @andyrj and @SkaterDad, then where I previously may have written

h('input', {
  type: 'submit',
  value: 'Submit',
  disabled: isFormBusy || !isFormValid
})

I should now write:

h('input', {
  type: 'submit',
  value: 'Submit',
  disabled: (isFormBusy || !isFormValid) ? 'disabled' : undefined
})

I guess it's reasonable. But I don't really have a problem with special handling of explicit boolean (not "booleany") values for "valueless attributes". In fact I kind of prefer it as it makes the code less noisy.

[rajaraodv]

As per w3c every attribute has value. If you don't set one, it's set to "" and assumed to be true.

Note:
A boolean attribute without a value assigned to it (e.g. checked) is implicitly equivalent to one that has the empty string assigned to it (i.e. checked=""). As a consequence, it represents the true value.
Note:
The values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether.

So when you pass h('button', {disabled: true}, [...]), ideally, Hyperapp should generate: <button disabled>...</button> or <button disabled="">...</button>. But I think most browsers ignore that w3c standard of not allowing "true" or "false". So generating <button disabled="true">...</button>, while not ideal, is also fine.

If we pass h('button', {disabled: "something"}, [...]), it should generate: <button disabled="something">. This will also result in a disabled button.

If we pass h('button', {disabled:0}, [...]), then I think we should generate : <button disabled="0">..will also result in a disabled button.

Similarly when you pass h('button', {disabled: false}, [...]), Hyperapp should generate: <button disabled="false">...</button>

We can add functionality where h('button', {disabled: null}, [...]) or h('button', {disabled: undefined}, [...]) will also result in removing that attribute.

[zaceno]

@rajaraodv

But when you pass h('button', {disabled: false}, [...]), Hyperapp should generate: <button>...</button> because any other value will be considered as true

Oh, I misunderstood you then. I thought you wanted disabled: false to result in <button disabled="false">..</button> -- which of course would be the opposite of the programmer's intent. (Because the button would be disabled)

I'm also with you on the null and undefined -- they should be treated the same as false. I also think "" should be removed, but not 0.

[rajaraodv]

Oh, I misunderstood you then. I thought you wanted disabled: false to result in .. -- which of course would be the opposite of the programmer's intent. (Because the button would be disabled)

@zaceno I am edited my earlier comment. You are correct in your original understanding that I actually want <button disabled="false">..</button> when h('button', {disabled: false}, [...]) is passed. Sorry for the confusion.
But people should be aware that only 'null' or 'undefined' is what they should send if they want to remove that attribute. Everything else will resulting in button being disabled.

[zaceno]

@rajaraodv Ah, ok -- good we understand eachother at least. 😅 Anyhow I'm still on the side that explicit false attributes should be removed.

If, for whatever reason, a programmer wants <x disabled="false">... they can set {disabled: "false"}

But people should be aware that only 'null' or 'undefined' is what they should send if they want to remove that attribute. Everything else will resulting in button being disabled.

The way I see it, this is one of those things where it won't matter how much we document this -- everyone will make this mistake. It's so natural to expect {disabled: false} means "not disabled". I see no reason not to go along with that.

undefined attributes should be removed of course -- no question there.
null... maybe. Not sure. At least I don't think it would be a problem if we do remove them.
"" should probably be removed in case you were building the attribute with some string-building function which returns an empty string to signify that there should be no value. At least, I don't see it ever being useful to make an attribute with an empty string as value.
0 is the only falsy value I think maybe we should not remove attributes for. It's probably a common expectation that any number value will be cast to a string. Including zero.

[rajaraodv]

@rajaraodv Ah, ok -- good we understand eachother at least. 😅 Anyhow I'm still on the side that explicit false attributes should be removed.

You are right. I just checked how React does this and they use explicit false to removes the attribute.

[lukejacksonn]

Great summary I think there @zaceno

false - remove
undefined - remove
null - remove
"" - remove
0 - set as string value

[thysultan]

Using an empty string "" as a remove signal might not work as expected for input.value.

[zaceno]

@thysultan that's a good point!

Maybe it's best to just leave string values as is, empty or not.

So:

• typeof attrVal === 'string' - leave as is.
• typeof attrVal === 'number' - cast to string
• else if !attrVal - remove

[jorgebucaran]

The current behavior will remove attributes when the value is "" and I haven't found any problems using inputs, so I am a bit confused.

[thysultan]

Case no.1 would produce Incorrect behavior in the following example:

var input  = document.createElement('input')
input.value = "value"

// 1
input.removeAttribute('value')
console.log(input.value) // "value"

// 2
input.value = ""

[jorgebucaran]

@thysultan Ah, you are right, right! 😅

We already handle "stateful" props like value and checked specially here.

[thysultan]

Yes, now that i look at how it's handled here, it probably shouldn't matter for the mentioned case(unless the logic of setElementProp changes) since app.js#L134-L143 executes both the setter and setAttribute operations.

Edit:

executes both the setter and setAttribute operations

The order that it executes them in means that it might not show in devtools, i.e if value = 0

[infinnie]

Even a literal value undefined could not remove certain properties like value, which causes the result that even an intention to remove the value attribute/property would not have the desired effect, but set the value of the element to undefined. See: https://codepen.io/anon/pen/mBoErw?editors=0010

Example code:

const { h, app } = hyperapp
/** @jsx h */

app({
  state: {
    test: false
  },
  view: (state, actions) =>
    state.test
        ? <div><input value="test"/><button onclick={actions.test}>Click me</button></div>
        : <div><input/><button onclick={actions.test}>Click me</button></div>,
  actions: {
    test: function (state) {
      return { test: !state.test };
    }
  }
})

[jorgebucaran]

@infinnie @rajaraodv Still not published, but both issues (value=0 and undefined) are now fixed in master!

https://github.com/hyperapp/hyperapp/blob/e02342b8f2ede9e655fcef406908ac536f904720/src/app.js#L146-L153

💪😄