Ignore undefined/falsey properties on components and elements

[TehShrike]

Brought up in chat by @arxpoetica: when using spread to apply an object's properties to an element, it is desirable to be able to say "apply everything except these specific properties".

Say, for example, something like <svelte:component this={MyComponent} {...all} />, which would result in a MyComponent property being set on the component.

If the spread operator ignored falsey values (or more safely, just ignored undefined values) on the object, it would be relatively easy to use something like Object.assign to specifically exclude some properties, say with this computed property:

computed: {
  forSpread: all => Object.assign({}, all, { MyComponent: undefined })
}

[dxlbnl]

I'd advice against changing the semantics of spread. I think comprehensions in js could have solved this issue: {...{key: value for key, value in obj if value}}. But it didn't land.
So I'd still advice for making a new object with it's properties filtered using computed.

[Rich-Harris]

I think the proposal in Gitter wasn't spread-specific, but would rather apply here as well:

<div aria-hidden={hidden}>...</div>

That currently renders as <div aria-hidden="false"> which is incorrect.

If we removed attributes with falsy values other than "", that would probably count as a breaking change. If we only removed attributes that were undefined, it probably counts as a bugfix.

[TehShrike]

It may be bad practice, but I do expect to be able to write styles using selectors like [some-state=false], so I'd rather the new behavior only applied to undefined.

[arxpoetica]

I'm leaning toward @TehShrike's answer, especially since one could still get undefined in an attribute through a string:

// foo = "undefined"
input: <elem class={foo}/>
output: <elem class="undefined"/>

// bar = undefined
input: <elem class={bar}/>
output: <elem/>

input: <elem class="undefined"/>
output: <elem class="undefined"/>

input: <elem class=undefined/>
output: <elem/>

[TehShrike]

I like undefined being ignored for all attributes, not just spread. It's more consistent. 👍

[kaisermann]

I also like undefined being ignored. In some cases, the browser resolves undefined to some undesirable things, like with the maxlength attribute.

About ignoring falsy values, I think false and 0 shouldn't be ignored.

[TehShrike]

To clarify: is the correct implementation of this proposal (to ignore all properties with value undefined) to make the change in the set code?

I thiiiink that might be my preference, so I could do

component.set(Object.assign({}, someObject, { ignoreThisProperty: undefined ))

but that might break everything for folks who actually do want to set values to undefined inside component code, so that might not be viable.

The alternative would be to change update to remove all undefined properties before calling _set on children.

[Rich-Harris]

Huh. As of 2.15, undefined and null attribute values are unset, though I've just realised that it doesn't make a difference in most cases since we use property access instead of setAttribute:

div.className = ctx.foo;

So I guess we should reopen this?

[PaulMaly]

Maybe be should also ignore NaN values?

[Conduitry]

I was having a look at this earlier today. As mentioned above, the problem is that for many attributes, we are updating them via their corresponding properties. So instead of setAttribute(div, 'class', whatever) (which would remove the attribute if whatever were undefined or null), we have div.className = whatever (which does not do that).

As far as I know, there is no way to remove an attribute via operations to its corresponding prop. We'd need to ad an extra check for each of these, and either remove the attribute or update the prop as appropriate.

It did make me wonder, though: What was the original purpose of using props when possible? This has been present since 1.0.0. Was it for performance reasons? Code size reasons? Knowing that would help with deciding how to implement this.

[warmrobot]

Can anybody suggest any suitable workaround for this issue?

[PaulMaly]

Seems, this issue still relevant for v3. Any chance it to be fixed in the nearest future?

[warmrobot]

It did make me wonder, though: What was the original purpose of using props when possible? This has been present since 1.0.0. Was it for performance reasons? Code size reasons? Knowing that would help with deciding how to implement this.

It seems, that Vue use attributes:
https://github.com/vuejs/vue/blob/61c32cc6732aaf225d868cee79872d9d9fdd5dcc/src/platforms/web/runtime/modules/attrs.js#L61
with some checking
from here
https://github.com/vuejs/vue/blob/61c32cc6732aaf225d868cee79872d9d9fdd5dcc/src/platforms/web/util/attrs.js#L22

[Conduitry]

For now, using spread attributes allows you to control which attributes appear on an element. <div {...foo}> where foo is an object of attribute keys and values will add/remove attributes according to which are present and non-null. This is definitely a workaround though, and it remains to be seen what's the best way to handle this with regular attributes.

[flekschas]

I know this is not a pressing issue but it'd be nice to remove attributes when the value is undefined, simply because most other frameworks I've dealt with because have similar functionality. It's good to know that the spread operator is a workaround but it takes some effort to find this out. I don't have anything new to bring to the table but someone on discord mentioned to close this ticket and I want to let you know that there are people still running into this issue :)

[RedHatter]

Just ran into this issue today. Adding my vote to have it fixed.

Was it ever determined whether using setAttribute would have any downsides?

[Conduitry]

This morning I was looking at this again and asked @lukeed whether he had any performance or other ideas about it.

He said in the past he had written a view library that actually used attributes in most cases - all of them except for 'id', 'key', 'nodeValue', 'textContent', 'className', 'innerHTML', 'tabIndex', 'value', 'style'. There isn't really a performance penalty for using the attribute form over the property form, and sometimes it's faster.

What this means for us I think is that we should comb through everything in the attribute_lookup object and see which are really necessary. This seems a bit daunting, and for the vast majority of them, I just plain old have no idea whether it's important that they continue to be set via properties. If anyone is able to take a look here and help out, that would be greatly appreciated.

[larrikinventures]

Fwiw, the spread workaround didn't work for me. It still set pattern="undefined"

Can repro with <input {...{pattern: undefined}}>.

I ended up using Lodash omitBy and isUndefined. E.g. <input {...omitBy({pattern: undefined}, isUndefined)}>.

[guilhermeocosta]

For now, using spread attributes allows you to control which attributes appear on an element. <div {...foo}> where foo is an object of attribute keys and values will add/remove attributes according to which are present and non-null. This is definitely a workaround though, and it remains to be seen what's the best way to handle this with regular attributes.

@Conduitry How exactly can I do it by passing props from an external component to a component which spreads it's props?

In my REPL example, assigning a value to the object prop reassign all it's content, omitting non-passed properties.

[kaisermann]

@guilhermeocosta You're overriding the settings prop entirely. You can create a computed property or separate that settings object into multiple props

Computed example

[guilhermeocosta]

Thanks @kaisermann, I've ended up doing something very similar with your REPL.

[RedHatter]

I've done some extensive research and experimentation. Here are my results.

My findings

How these result were obtained

• Careful examination of the specs for IDL attribute reflection
• Reading the specs for the specified content attribute and comparing to the IDL attribute (the property)
• In browser testing of a random selection of attributes as well as any attribute that has unique features or a non-standard IDL attribute type

Unique attributes

I only found two attributes in the list that required special handling.

• indeterminate
  Doesn't have an associated attribute can only be set via property.
• buffered
  Read only property doesn't have an associated attribute and can't be set at all. Probably shouldn't be on the list.

Boolean attributes

The handling of boolean attributes is some what unique among attribute types. If the property is set to any value other than false the then corresponding attribute is set to "" while if the property is set to false then the attribute is removed from the element. This means that e.g. element.setAttribute("disabled", false) actually sets the disabled property to true.

One note is that contentEditable is not a boolean attribute.

novalidate: { property_name: 'noValidate', applies_to: ['form'] },
allowfullscreen: { property_name: 'allowFullscreen', applies_to: ['iframe'] },	
sandbox: { applies_to: ['iframe'] },
async: { applies_to: ['script'] },
defer: { applies_to: ['script'] },
autofocus: { applies_to: ['button', 'input', 'keygen', 'select', 'textarea'] },
multiple: { applies_to: ['input', 'select'] },
readonly: { property_name: 'readOnly', applies_to: ['input', 'textarea'] },
required: { applies_to: ['input', 'select', 'textarea'] },
reversed: { applies_to: ['ol'] },
selected: { applies_to: ['option'] },
open: { applies_to: ['details'] },
controls: { applies_to: ['audio', 'video'] },
autoplay: { applies_to: ['audio', 'video'] },
loop: { applies_to: ['audio', 'bgsound', 'marquee', 'video'] },
muted: { applies_to: ['audio', 'video'] },
spellcheck: {},
draggable: {},
default: { applies_to: ['track'] },
checked: { applies_to: [
	'command',
	'input'
] },
disabled: {
	applies_to: [
		'button',
		'command',
		'fieldset',
		'input',
		'keygen',
		'optgroup',
		'option',
		'select',
		'textarea',
	],
},
ismap: { property_name: 'isMap', applies_to: ['img'] },
contextmenu: {},
scoped: { applies_to: ['style'] },
seamless: { applies_to: ['iframe'] },

All other attributes

All other content attributes reflect the underlining IDL attributes exactly when setting, blindly setting the corresponding content attribute. getting the value is quite another story, many properties sanitize or transform the value before returning. This doesn't matter in our case since we only care about setting but I thought I'd mention it.

Conclusion

Based on the above we should be safe to use setAttribute provided we handle indeterminate and boolean attributes correctly. We can't simply remove any attribute when it equals false as that is a valid value in some cases, but it should be easy enough to maintain a list of boolean attributes for special handling. After all we already do.

Edit: One other note. Not all browsers set the value content attribute when the property is set, it shouldn't matter as the property is still set when the attribute is set, but I'll add it to the list anyway in case it has performance implications and shouldn't cause problems with undefined.

[Conduitry]

Thank you so much for doing all this @RedHatter!

[Conduitry]

Using that list of attributes, but also adding back in contenteditable, indeterminate, and value, all unit tests pass, apart from ones testing for specific js output.

contenteditable - I'm not sure what's happening with this right now. There was something else related to binding to contenteditable elements that was recently merged prematurely, and there's another PR from Rich that's intended to flesh some of this out. I'm going to kick the can on this one for now.

indeterminate - Even if this is not actually a valid attribute, there is currently a test assuming that it is, and there wouldn't really be any other reasonable way for people to specify the boolean property on an element without having some sort of invented attribute, I think this should stay.

value - Leaving this out, so that it would be set as an attribute instead of as a property, broke a number of unit tests. I can't tell how many of these are valid test failures, and how many are tests being too narrow or maybe are us bumping into JSDOM limitations. I'm honestly a little surprised that setting the value attribute on an input field does update it - I thought I had had that trained out of me by jQuery's .attr() vs .prop(). But anyway I see that as I am writing this, the post I'm responding to is being updated, and it now recommends that value continue to be set as a property, so, there we go I guess.

[RedHatter]

contenteditable - Looking at the failures it seems to be a problem with jsdom

const { JSDOM } = require("jsdom")
const dom = new JSDOM("<p></p>")
dom.window.document.querySelector("p").contentEditable = true
assert(dom.window.document.body.innerHTML == '<p></p>')

dom.window.document.querySelector("p").setAttribute("contentEditable", true)
assert(dom.window.document.body.innerHTML == '<p contenteditable="true"></p>')

Functionally they're the same, jsdom just isn't reflecting the IDL attribute like it should. When the expected output is modified to include the attribute the tests pass fine. This leads me to believe that the binding features still work as they should and the failures are superficial.

indeterminate - I'm sorry if I wasn't clear. I intended to say that indeterminate should be kept in attribute_lookup. It can't be set via setAttribute therefore we need to set it with a property. In other words: I agree.

[cvlab]

I want to reopen this Issue because the spread operator is less usable than I'd like it to be.

1. There is no way to filter/delete an attribute Object destructed props did not match sometimes #2282 it became undefined
2. undefined or null are not deleted, so title became title="undefined", pattern="undefined"

https://svelte.dev/repl/cb2ad72a9e14443ea6c3deeb2c7b8c13?version=3.6.9

My guess, easiest way to resolve is to remove undefined and null values in spread operator with element.removeAttribute(attrName), but maybe there are cases when element[attrName] = undefined is expected behaviour?

Thanks!

[joelparkerhenderson]

Any update? I am hitting what seems to be the same kind of issue: I have a button that uses disabled={false} and Bulma CSS is disabling the button. Thanks!

[Conduitry]

3.13.0 fixed a lot of niggling bugs with spread attributes. It looks like @cvlab's REPL example above is working now.

@joelparkerhenderson Do you have a repro? If it only looks like a button - but is not actually one of the elements that the spec says has disabled as a boolean attribute - then false is going to get coerced to a string. If you need a non-boolean attribute that is conditionally present, you can use null or undefined.