svelte: Ignore undefined/falsey properties on components and elements

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 })
}

About this issue

  • Original URL
  • State: closed
  • Created 6 years ago
  • Comments: 28 (23 by maintainers)

Commits related to this issue

Most upvoted comments

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.

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.

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.

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.

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 😃

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.

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.

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

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

Computed example

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?