Rich-Harris commented 1 month ago

Describe the problem

One of the many reasons custom elements are a pain in the neck is that it's often rather ambiguous what's supposed to be an attribute and what's supposed to be a property. Some custom elements 'reflect' attributes back to properties and vice versa, but there's no universally agreed upon convention. If you're doing low-level DOM manipulation you can decide whether you want to do element.setAttribute(name, value) or element[name] = value, but everyone stopped writing code like that a decade ago.

It's a messy, flawed design, and different frameworks deal with it in different ways. Preact and Svelte both treat something as a property if prop in node, and an attribute otherwise. Vue does the same thing but additionally gives you a way to force something to be a prop (I don't think you can force something to be an attribute) with some interesting syntax choices:

Lit leans even harder into syntax, with ? and . and @ prefixes for boolean attributes, properties and event handlers respectively:

I think the explicit control is a good thing, but I think we can provide it more naturally.

Describe the proposed solution

7925 and #12479 point the way. We're moving to a world in which quoted props on components are stringified:

<script>
  let object = { a: 1, b: 2, c: 3 };
</script>

<!-- in Svelte 6, this will mean `data="[object Object]"` -->
<Thing data="{object}" />

(The reason the quotes are ignored today is historical: in the distant past before Svelte editor extensions, when components were typically written in .html files, it avoided crazypants syntax highlighting.)

What if we did something similar for custom elements?

<foo-bar someprop={somevalue} someattribute="{someothervalue}"></foo-bar>

In other words if something is quoted, it's an attribute, otherwise it's a prop. We might need to change some Prettier defaults, but other than that this should be a relatively straightforward change. (I don't see any need to distinguish between 'boolean attributes' and properties like Lit does — they're all just properties.)

It is, of course, a breaking change. We would need to decide whether to do this in 5.0, or have a bunch of warnings and change it in 6.0. Ordinarily the conservative thing to do would be to delay the breaking change, but since it's not currently possible to differentiate properties and attributes I think there's a reasonable case for doing it sooner. As such I've given it a 5.0 milestone for now.

Importance

nice to have

trueadm commented 1 month ago

I actually love this a lot. Simplifies a ton of things along the way too.

MarcusOtter commented 1 month ago

And I assume to pass a string as a property the syntax would be this? (if I don't have it in a variable)

<foo-bar someprop={"my string here"}></foo-bar>

ottomated commented 1 month ago

My only qualm with this is that it seems hard to teach and unintuitive. Why does wrapping something in quotes mean that it's an attribute?

henrikvilhelmberglund commented 1 month ago

My only qualm with this is that it seems hard to teach and unintuitive. Why does wrapping something in quotes mean that it's an attribute?

Because HTML attributes use quotes style="color:red;". Imo makes perfect sense, if it has quotes outside of {} eg. "{}" it's an attribute (like normally in HTML), if it does not it's a prop.

ottomated commented 1 month ago

if it has quotes outside of {} eg. "{}" it's an attribute (like normally in HTML), if it does not it's a prop.

But in the case <foo-bar prop="foo" />, prop would be a property, right? What about in the case <foo-bar prop="{foo} a b" />?

To me, "{}" just signifies string interpolation, which is why I think that getting rid of the confusing legacy behavior is great – I don't like replacing it with a confusing new behavior.

It's kind of like if `${foo}` had a special behavior in javascript.

Rich-Harris commented 1 month ago

<foo-bar prop="foo" /> — prop is an attribute, because "foo". If you want a property, prop={"foo"}

<foo-bar prop="{foo} a b" /> — also an attribute

ottomated commented 1 month ago

What would the recommended way to do string interpolation for a property? <foo prop={`${foo} a b`} />?

Rich-Harris commented 1 month ago

Exactly, just like in normal JavaScript:

props = {
  prop: `${foo} a b`
};

MrHBS commented 1 month ago

Yes I love this ❤️

dummdidumm commented 1 month ago

What about spreading properties? Does this always count as setting them as properties?

What about regular elements? I've seen (very few, rare) cases where people wanted things explicitly as an attribute or property.

As for timing: Since this is an uncommon case and can easily solved through e.g. an action, I'm in favor of taking our time with it and don't rush this into 5.0

rChaoz commented 1 month ago

How about boolean attributes? REPL

<input type="text" disabled={disabled}>
<!-- or -->
<input type="text" {disabled}>

Also, I believe it will be very easy to confuse the following, especially for new users:

<my-element a="{variable}" b={"text"} />

If we are going to make a breaking change, why not just force props to use a prefix, like :, or the same for attributes:

<my-element a={variable} :b="text" />

...and the same rules we knew so far about {} apply just the same.

Side now: I totally agree with arg="{variable}" converting the argument to a string, I wouldn't mind seeing that sooner than latter, but it would still be pretty breaking.

Theo-Steiner commented 1 month ago

I've been meaning to file an issue for this for ages, but never got around (totally not me bumping into this last year and promising to file an issue for it... 😅 )

Vue does the same thing but additionally gives you a way to force something to be a prop (I don't think you can force something to be an attribute) with some interesting syntax choices:

there's actually a syntax in vue for explicitly attributes as well: <my-foo bar.attr="passed as attribute"></my-foo> (although I don't think it is really documented anywhere?)

I think if it is set in quotes, it is an attribute, otherwise it's a property is a very elegant solution, but a few issues came to mind

it's quite hard to spot
it breaks with expecations of svelte 4 users
having an equivalent to the prop shorthand <my-foo {bar}/> would be problematic
- for bar="hidden", <my-foo "{bar}" /> will be translated to which one? a. <my-foo hidden="hidden" /> b. <my-foo hidden />
one shortcoming that came to mind:
- since {"foo": "bar"} is equivalent to {foo: "bar"}, spreading can only ever set props
- in vue, one could theoretically spread {"foo.attr": "bar"} vs {"foo": "bar"} to explicitly set a prop
- (I don't think this is implemented in vue though)

Since this is an edge case, trading a less elegant/simple API for explicitness / programmatic control could be worth the tradeoff? Maybe not reinventing the wheel, but picking either lit's .foo & .foo? or vue's foo.prop & foo.attr actually could be a good solution here, since people are already familiar with it and it wouldn't conflict with the existing way of doing things

Rich-Harris commented 3 weeks ago

What about spreading properties? Does this always count as setting them as properties?

There's basically three options:

{...stuff} spreads props
{...stuff} spreads attributes (or rather, does what set_attributes does on regular elements)
It uses the current heuristic

Given that a custom element has a finite number of props and a potentially infinite number of attributes, I would argue that (1) doesn't make sense — if you want to carve the props out of stuff, you can easily do that. That also results in the desired behaviour when spreading things like class.

I think (2) is the best option — it's consistent with how normal elements are handled, and easier to explain. We could augment it with a 'did you mean to do <foo-bar baz={stuff.baz} />?'-type warning, which we could also use to help people make sure they're using the right thing when not spreading. This would alleviate the concerns around discoverability.

How about boolean attributes?

For consistency I think we'd want to treat disabled="{disabled}" as an attribute in the context of a custom element (but this would raise the sort of runtime warning I just described, if the element in question implemented a disabled prop, or it was a property on HTMLElement.prototype like hidden).

This does create an inconsistency with <input disabled="{disabled}">, because we always treat that as a property (because there's no ambiguity when dealing with known properties of built-in elements). But I think we could separately consider compiler warnings in those cases, nudging people towards disabled={disabled} (or {disabled}) instead.

Maybe not reinventing the wheel, but picking either lit's .foo & .foo? or vue's foo.prop & foo.attr actually could be a good solution here

Thanks, I hate it 🤪

foo.prop and foo.attr are just terrible syntax IMHO — dot notation already means something in JS, and this looks like we're setting the attr property of the foo object or something. Really feels like a hasty compromise that was necessitated by constraints we don't share (like being able to read templates from HTML). .foo isn't quite as egregious, but it's still an ugly anomaly when it's only required for custom elements. Aesthetics matter!

I do think that if we started to nudge people towards a similar distinction for normal elements (by warning on quoted boolean properties like <input disabled="{disabled}">) it would soon feel like second nature, though I think the proposal still has value without that.

Rich-Harris commented 3 weeks ago

Just checked what we currently do...

<a-b x={1} y="2"></a-b>
<c-d {...stuff}></c-d>

...and we use set_attributes for spread props on custom elements, i.e. option (2). Past us clearly agreed at least with that part of the reasoning. So I guess this proposal basically boils down to this:

export default function App($$anchor) {
  var fragment = root();
  var a_b = $.first_child(fragment);

- $.set_custom_element_data(a_b, "x", 1);
- $.set_custom_element_data(a_b, "y", "2");
+ $.set_custom_element_property(a_b, "x", 1);
+ $.set_custom_element_attribute(a_b, "y", "2");

  var c_d = $.sibling(a_b, 2);
  let attributes;

  $.template_effect(() => attributes = $.set_attributes(c_d, attributes, { ...stuff }, undefined, true));
  $.append($$anchor, fragment);
}

...along with an implementation of set_custom_element_property and set_custom_element_attribute that issues an ignorable warning if it looks like you picked the wrong form.

dummdidumm commented 3 weeks ago

Yes and no - set_attributes does check if there's a property on the element and uses that, and only falls back to set_attribute if there isn't. set_custom_element_data does the same.

This distinction between custom elements and regular elements and how string attributes are set will require a lot of explanation, and I'm not sure the nuance is easily grasped. If anything, this makes me lean towards aligning it and meaning "string attributes will always be set through set_attributes, regardless of whether it's a custom element or a regular element". But this would be a pretty huge breaking change, which we cannot do on a whim. And people might have learned from past experiences that there's no difference, and memory muscle will make them use quotes when they shouldn't, and because there's a difference now prettier will no longer format on into the other. Even though it looks a bit ugly I therefore also lean towards a prefix, making use of our existing : syntax: attr:... and prop:... (which you could also use within spreading, like { 'attr:...: ..., 'prop:...': ... }`). The use cases for this are rare enough that a bit more to type is worth the improved readability.

Rich-Harris commented 3 weeks ago

which you could also use within spreading, like { 'attr:...: ..., 'prop:...': ... }`

That strikes me as very odd, to be honest. It's a big departure from how directives normally work...

<!-- one of these things is not like the others! -->
<CustomElementWrapper on:eventname attr:foo prop:bar />

...and it gets particularly weird if you want to reference the value inside the wrapper for whatever reason:

let props = $props();

// i can easily imagine people being confused as to why `props.foo` and `props.bar` are undefined
let foo = props['attr:foo'];
let bar = props['prop:foo'];

Rich-Harris commented 3 weeks ago

Opened #12981 so we can try this out

dummdidumm commented 3 weeks ago

You're right, spreading is probably best handled through a option 2 then, but my point about readability stands

PatrickG commented 3 weeks ago

Moving from <img src={someSrc} /> to <special-img src={someSrc} /> might be confusing.

Rich-Harris commented 3 weeks ago

There are three possibilities:

<special-img> has a src accessor and no src attribute — it works as expected, no changes required
<special-img> has both a src accessor and an attribute ('reflected', in the jargon) — it works as expected, no changes required
<special-img> has a src attribute that is not reflected, in which case Svelte will print the warning telling you that it needs to be an attribute rather than a property

So in the worst case — 3 — you're guided towards the correct outcome and you're learning something about how the element is implemented.

To me the more interesting case is 2. Maybe you need src to be an attribute, because you have some CSS like this...

special-img:not([src]) {
  outline: 5px solid red;
}

...or because you need it for document.querySelector(...) or whatever. Today, it's impossible (at least without an action, which is cumbersome) — Svelte would assume src was supposed to be a prop, because a src prop exists, and would omit the attribute.

With this proposal, it's trivial. As far as I'm concerned it's all upside.

PatrickG commented 3 weeks ago

Sounds good. And how would the SSR output look like? I guess it would be handled like a normal html element, so there would be a src attribute, right?

Rich-Harris commented 3 weeks ago

That's a great point, I didn't really think about SSR in the PR but it's another point in this proposal's favour. At the moment we have to assume everything is an attribute, even if that results in prop="[object Object]". That's no longer true (except for spread attributes, where we still have to make that assumption). Will update the PR

dummdidumm commented 2 weeks ago

What do we do about dash-case? It's somewhat common to use dash-case on attributes and convert them into camelCase on the corresponding attribute.

<c-e a-property={true}></c-e>

You likely don't want this as cE['a-property'] = true. On the other hand, with $props() it's somewhat straightward to define props like that (let { 'a-property': aProperty } = $props(). Should that mean that we have a ['a-property'] property? Do we also create a aProperty automatically?

Rich-Harris commented 2 weeks ago

We decided to punt on this until 6.0

dummdidumm commented 2 weeks ago

@trueadm said that React had a similar syntax to distinguish between properties and attributes way back, and it was very confusing to people when to use which. This may happen here aswell, especially given that the quotes have slightly different meanings on other kinds of elements and components.

sveltejs / svelte

Distinguish between properties and attributes on custom elements #12624

Describe the problem

Describe the proposed solution

7925 and #12479 point the way. We're moving to a world in which quoted props on components are stringified:

Importance