-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
warn on <svelte:element this="literal">
#11398
Comments
IIRC we at some point had at least semi-blessed using this as a workaround for people who wanted actual |
It looks like what I'm referring to is #10641 and it also sounds like we have a way of addressing this in Svelte 5, but it also looks like this didn't make it into https://svelte-5-preview.vercel.app/docs/breaking-changes |
Yeah, even in cases where we don't preserve <svelte:element this={'slot'} /> {@html '<slot></slot>'} So I don't think that should influence our decision. |
I've seen people use |
That's definitely not something we want to bless, since the behaviour is pretty counterintuitive — this... <svelte:element this="script">
console.log({ count });
</svelte:element> ...will log Regardless, anyone foolhardy enough to do that (rather than just putting them inside wrapper elements, in which case their contents will be interpreted literally) still has the |
I think we should just leave this as is. There's no point in adding more code to warn against something that somebody can then work around with |
I checked what Svelte 4 does, and it turns out that it treats static values specifically, basically resulting in as of you wrote |
The fact that these behave completely differently... <svelte:element this="input" bind:value={name} /> <svelte:element this={"input"} bind:value={name} /> ...is itself a reason to change the behaviour. It's extremely confusing and serves no purpose. I definitely don't think we need to make Svelte 5 behave like Svelte 4 in this regard. I could only find two people on GitHub using static
Doubtless there are more occurrences than can be found in public repos with grep.app, but we're talking about a change that will affect vanishingly few people, while getting rid of some needless complexity. |
I can get behind not replicating behavior, but why add a warning? It also adds needless complexity to the code base. If people want to do that for some reason we just leave them be. |
😅 I haven't touched this code in a good while, but that's a bug. It originally was a dynamic element across header levels (for accessibility/page hierarchy purposes), but seemingly got removed and never added back in a refactor. You have my blessing to break or warn on this as you wish, lol. |
😅 After the discussion with @dummdidumm on #10130 I think I'll be dropping what you've mentioned here @Rich-Harris 👍 |
Au contraire, the diff is mostly red: #11454
It feels very weird to have a breaking change ( |
Why is it strange that
Well, of course it is, because the bulk is removing parts of that static behavior. If you didn't add the warning it would be even more red. |
When you say 'I can get behind not replicating behavior' you're explicitly advocating for a breaking change! Just one that's less explicit and obvious how to fix.
Of the two people we've identified that are doing this, 0% say it's important that we keep this. |
The main use cases for static strings are having top level styles/scripts or slots treated as regular elements and not getting special behavior. Those people would now have to do If this is such an important thing for you to discourage it, can we at least only warn and error only in Svelte 6? I just want to avoid any kind of unforced breaking changes for v5. |
Can we migrate this automatically by migration tool? <svelte:element this="div" /> will be <svelte:element this={"div"} /> I remember the added complexity to the compiler when static strings were supported in Svelte 3. I don't think there are any use cases where static strings have to be used. I upvote that migrating them in Svelte 5 by migration tool and throw a warning by compiler , and making it a compile error in Svelte 6. |
I think it's absolutely the wrong decision. With #11454, we have a clearly signposted breaking change that is easy to fix and that will affect approximately zero users, and we can simplify the codebase as a bonus. Nevertheless, I've opened #11641 since there's resistance to this approach. Reasons we should close that PR and merge #11454 instead:
|
I vote for removing the code around special-casing this-strings like in #11454 but only give a warning. |
That's what #11641 is, no? |
(note that it's a PR against #11454, not |
Describe the problem
While reviewing #11387 i realised we have some logic for analysing literal tagnames in cases like this:
This is utterly pointless. There is no situation in which you should be using a static value with
<svelte:element>
. The only thing I can think of that resembles a use case is where you have a preprocessor of some kind, in which case the preprocessor should just be creating the right tag in the first place.Describe the proposed solution
Make
<svelte:element this="...">
a compiler error, and delete any code paths that depend on it being a literal value.(Of course, people could still do
<svelte:element this={"..."}>
, but that's not a reason to encouragethis="..."
.)Importance
nice to have
The text was updated successfully, but these errors were encountered: