svelte icon indicating copy to clipboard operation
svelte copied to clipboard

breaking: use quotes to differentiate custom element properties from attributes

Open Rich-Harris opened this issue 1 year ago • 6 comments

Closes #12624 — I figured it would be worth having a branch to play with so that we can give it a proper spin.

Only two tests needed to be updated. The attribute-custom-element-inheritance test was very easy to fix because the warnings told me exactly what I needed to do. The other one was a tiny bit trickier because the attribute is reflected as a prop (squelching any warning) but needs to be present as an attribute in order to influence the CSS. Definitely a bit of an edge case, but flagging it nonetheless (and the "{red || undefined}" is admittedly weird).

On the whole, I like it — it gives control that doesn't exist today, and guides people towards the correct outcomes except in rather esoteric cases. I definitely like it more than ideas like attr:foo and prop:bar for the reasons given here: https://github.com/sveltejs/svelte/issues/12624#issuecomment-2305638729

Dynamic custom elements are currently a TODO (though the current heuristic obviously works fine for the existing tests)

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

  • [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • [x] Prefix your PR title with feat:, fix:, chore:, or docs:.
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • [x] Run the tests with pnpm test and lint the project with pnpm lint

Rich-Harris avatar Aug 22 '24 21:08 Rich-Harris

🦋 Changeset detected

Latest commit: 6226b8545b381d3ae74c89bc98922c8deb4016a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Aug 22 '24 21:08 changeset-bot[bot]

Am on board with making this a runes-mode-only thing

I'm also wondering if we should keep the "no property given, set as attribute instead" behavior

Not sure I understand?

we also need to update prettier-plugin-svelte

Mind if I leave that to you? I'm unfamiliar with the codebase, so it'll be quicker that way...

Rich-Harris avatar Aug 27 '24 12:08 Rich-Harris

I'm also wondering if we should keep the "no property given, set as attribute instead" behavior

Not sure I understand?

I mean that <c-e foo={bar} /> doesn't mean cE.foo = bar but still uses set_custom_element_data(cE, 'foo', bar), and the function checks "oh there is no property foo, so I'll set it as an attribute instead". That way you can mostly use that form without thinking about it too much, and you only need to use foo="{bar}" in case you explicitly want it as an attribute, even if it has a corresponding property.

Mind if I leave that to you?

Yeah no problem, was more a note to myself that we (I) need to adjust that if we go with this change.

dummdidumm avatar Aug 27 '24 14:08 dummdidumm

I mean that <c-e foo={bar} /> doesn't mean cE.foo = bar

If there's no foo accessor then you'll get a dev time warning suggesting that you do foo="{bar}" instead. I don't think we should go back to being loosey-goosey, because then you have no way to say 'actually I would like this to be assigned as a prop, and I don't care whether there's an accessor or not'.

For example you might have a prop that doesn't have a corresponding accessor because it doesn't affect rendering (e.g. it's only referenced inside an event handler). It's a rare case but it could happen. With the PR as it currently is you can just leave a note for yourself explaining that:

<!-- svelte-ignore custom_element_invalid_property — foo is a non-reactive property, not an attribute -->
<c-e foo={bar} />

Rich-Harris avatar Aug 27 '24 15:08 Rich-Harris

There's also a case where the property isn't defined yet if the custom element definition is being lazy-loaded -- so I agree with Rich that being explicit makes sense.

Old post of mine about that case: https://css-tricks.com/using-custom-elements-in-svelte/#aa-lazy-loading-custom-elements

geoffrich avatar Aug 27 '24 17:08 geoffrich

To be fair things would still be broken in that case — I wonder if we need to add some customElements.whenDefined logic to handle lazy-loaded custom elements. Annoying bloat in most cases of course, but that's the price you pay for using a broken primitive. Would be a separate chunk of work to this PR

Rich-Harris avatar Aug 27 '24 17:08 Rich-Harris

Closing as we decided to delay this until 6.0

Rich-Harris avatar Aug 30 '24 15:08 Rich-Harris