Setting prop using Boolean (to trigger prop over attribute) always sets true about incremental-dom

[ewinslow]

@sparhami It seems like the main reason this can't be changed is for backwards compatibility reasons. There is a very clear and cheap (and I'd argue, more correct) solution, which is to use the string value:

elementOpen('div', null, null, 'aria-expanded', 'true');

Or another case, where you just need attribute presence:

elementOpen('button', null, null, 'disabled', '');

But setting-boolean-properties have much more painful workarounds.

I really think setting properties for booleans is the more intuitive behavior here. All code that assumes otherwise can be trivially changed to use strings today and still work fine. But if it's just too much trouble, then I totally get that...

[iteriani]

It's kind of costly to always check for boolean. Is it really necessary to create Boolean objects?

[sparhami]

I'm not sure if the cost of the check is that great, but it is additional code to compile and ship. There is a cost to allocating the Boolean object however when using new, which applies to every diff regardless of whether or not there is a change.

I think a more promising approach would be to customize the behavior as described here: http://google.github.io/incremental-dom/#setting-values

[ewinslow]

For our project, we opted to change the default behavior of incremental-dom to always assign boolean primitives to the property. If you want to assign the attribute, you just have to convert to a string literal first. If you want to assign a string property, you just use "new String()".

[trevorparscal]

Thank you all for the ideas. I ended up making typeof boolean always use setProp, the same way typeof object and typeof function already do. This introduces a typeof check, but it's less costly than constructing the Boolean during the render plus doing an instanceof on the other side.

The customizing behavior seems like a nice idea, but I worry about bugs. If the library behaves differently from project to project in sneaky ways depending on if the same hooks are setup I suspect there will be tricky to diagnose problems from sharing code or practices.

[trevorparscal]

I've been playing around with this a bit more. The patch I'm using (based on ewinslow's comment) adds some code to applyAttributeTyped, changing...

if (type === 'object' || type === 'function') {

to...

if (type === 'object' || type === 'function' || type === 'boolean') {

This check is very inexpensive, requires no object construction on the calling side and solves the general case of elegantly setting a boolean as a property. It's also easy to document this behavior - just as we already document the same behavior for objects and functions.

The alternative suggested is something like this:

attributes.checked = applyProp;
attributes.selected = applyProp;
attributes.disabled = applyProp;
// and so on for readOnly, multiple, isMap, defer, declare,
// noResize, noWrap, noShade, compact, etc.

This is a bit complex just to get standard HTML elements to behave as expected. Another problem with hooks is that it introduces potential conflicts when used with other types of elements, such as SVG or shadow-dom, which either have non-boolean attributes or properties with similar names or have boolean attributes or properties not explicitly listed - or both.

Are we sure it's not worth adding the boolean typeof check to the core library to resolve this problem in a simple, general and consistent way?

[sparhami]

I've been playing around with this a bit more. The patch I'm using (based on ewinslow's comment) adds some code to applyAttributeTyped, changing...

if (type === 'object' || type === 'function') {

to...

if (type === 'object' || type === 'function' || type === 'boolean') {

This check is very inexpensive, requires no object construction on the calling side and solves the general case of elegantly setting a boolean as a property. It's also easy to document this behavior - just as we already document the same behavior for objects and functions.

The alternative suggested is something like this:

attributes.checked = applyProp;
attributes.selected = applyProp;
attributes.disabled = applyProp;
// and so on for readOnly, multiple, isMap, defer, declare,
// noResize, noWrap, noShade, compact, etc.

This is a bit complex just to get standard HTML elements to behave as expected. Another problem with hooks is that it introduces potential conflicts when used with other types of elements, such as SVG or shadow-dom, which either have non-boolean attributes or properties with similar names or have boolean attributes or properties not explicitly listed - or both.

Are we sure it's not worth adding the boolean typeof check to the core library to resolve this problem in a simple, general and consistent way?

We cannot change to use type === 'boolean'. Take for example:

let expanded = false; // This variable is set to a boolean value at some point.
...
// It now gets set as an attribute, and  converted to a string.
elementOpen('div', null, null, 'aria-expanded', expanded);

This cannot be set as a property and changing this will break existing users in a way that may not be easy to detect. I think for some uses this might not be a big concern, or they might want to have another way around like:

let expanded = false;
...
elementOpen('div', null, null, 'aria-expanded', toString(expanded));

or automatically do that a higher level construct that wraps elementOpen, but I think it all comes back to not using Incremental DOM directly, but rather configured in some sort of way for higher level usage.