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
- Merge branch 'master' into gh-1434 — committed to sveltejs/svelte by Rich-Harris 6 years ago
- Merge pull request #1815 from sveltejs/gh-1434 Don't render undefined/null attributes — committed to sveltejs/svelte by Rich-Harris 6 years ago
- only set attributes via properties when truly necessary (#1434) — committed to sveltejs/svelte by Conduitry 5 years ago
- only set attributes via properties when truly necessary (#1434) — committed to sveltejs/svelte by Conduitry 5 years ago
- only set attributes via properties when truly necessary (#1434) — committed to sveltejs/svelte by Conduitry 5 years ago
- update tests (#1434) (#2935) — committed to sveltejs/svelte by Conduitry 5 years ago
- update tests (#1434) (#2935) — committed to sveltejs/svelte by Conduitry 5 years ago
- only set attributes via properties when truly necessary (#1434) — committed to sveltejs/svelte by Conduitry 5 years ago
- update tests (#1434) (#2935) — committed to sveltejs/svelte by Conduitry 5 years ago
I’ve done some extensive research and experimentation. Here are my results.
My findings
How these result were obtained
Unique attributes
I only found two attributes in the list that required special handling.
indeterminateDoesn’t have an associated attribute can only be set via property.bufferedRead 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
falsethe then corresponding attribute is set to""while if the property is set tofalsethen the attribute is removed from the element. This means that e.g.element.setAttribute("disabled", false)actually sets the disabled property totrue.One note is that
contentEditableis not a boolean attribute.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
setAttributeprovided we handleindeterminateand boolean attributes correctly. We can’t simply remove any attribute when it equalsfalseas 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
valuecontent 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 withundefined.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 toundefined.I think the proposal in Gitter wasn’t spread-specific, but would rather apply here as well:
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 wereundefined, it probably counts as a bugfix.contenteditable- Looking at the failures it seems to be a problem withjsdomFunctionally 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 inattribute_lookup. It can’t be set viasetAttributetherefore 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
undefinedbeing ignored. In some cases, the browser resolvesundefinedto some undesirable things, like with themaxlengthattribute.About ignoring
falsyvalues, I thinkfalseand0shouldn’t be ignored.For now, using spread attributes allows you to control which attributes appear on an element.
<div {...foo}>wherefoois 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
settingsprop entirely. You can create a computed property or separate thatsettingsobject into multiple propsComputed example
Huh. As of 2.15,
undefinedandnullattribute values are unset, though I’ve just realised that it doesn’t make a difference in most cases since we use property access instead ofsetAttribute:So I guess we should reopen this?