svelte
svelte copied to clipboard
breaking: use quotes to differentiate custom element properties from attributes
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:, ordocs:. - [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 testand lint the project withpnpm lint
🦋 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
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...
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.
I mean that
<c-e foo={bar} />doesn't meancE.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} />
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
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
Closing as we decided to delay this until 6.0