Skip to content
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

Closed
Rich-Harris opened this issue Apr 30, 2024 · 20 comments · Fixed by #11454
Closed

warn on <svelte:element this="literal"> #11398

Rich-Harris opened this issue Apr 30, 2024 · 20 comments · Fixed by #11454
Assignees
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

While reviewing #11387 i realised we have some logic for analysing literal tagnames in cases like this:

<svelte:element this="svg">...</svelte:element>

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 encourage this="...".)

Importance

nice to have

@Rich-Harris Rich-Harris added this to the 5.0 milestone Apr 30, 2024
@Conduitry
Copy link
Member

IIRC we at some point had at least semi-blessed using this as a workaround for people who wanted actual <slot> elements in their components. Does that change anything for you?

@Conduitry
Copy link
Member

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

@Rich-Harris
Copy link
Member Author

Yeah, even in cases where we don't preserve <slot> but should (e.g. the <slot> element is in a component inside the template, so the compiler can't see it) it's still easy enough to work around:

<svelte:element this={'slot'} />
{@html '<slot></slot>'}

So I don't think that should influence our decision.

@7nik
Copy link

7nik commented May 1, 2024

I've seen people use this="script" and this="style" when they want to insert a script/style element into the markup.

@Rich-Harris
Copy link
Member Author

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 0 rather than { count: 0 }. And I'm not sure you can do anything useful with this="style" given the prevalence of {...} in CSS.

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 this={'script'} escape hatch.

@dummdidumm
Copy link
Member

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 {"x"}. If people do it they do it for a reason and requiring a useless expression for these cases just makes the task more cumbersome for no real gain.
At the same time we should check what Svelte 4 does currently. If it handles static values differently we should replicate that, if it doesn't we should delete all code that started treating it special.

@dummdidumm
Copy link
Member

I checked what Svelte 4 does, and it turns out that it treats static values specifically, basically resulting in as of you wrote <div></div> instead of <svelte:element this="div"></svelte:element>, along with all the a11y validations kicking in, bind:value being allowed on this="input" etc.
In the spirit of as few breaking changes as possible we should adjust the behavior accordingly.

@dummdidumm dummdidumm self-assigned this May 2, 2024
@Rich-Harris
Copy link
Member Author

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 this values:

  • @spences10, whose use case is easily satisfied by other means such as this={"script"} (side note: it's pretty weird that we make this="input" behave exactly like an <input>, but we don't make this="script" behave exactly like a <script>
  • @Tropix126 who does this="h" for reasons I don't totally understand

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.

@dummdidumm
Copy link
Member

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.

@Tropix126
Copy link

Tropix126 commented May 2, 2024

@Tropix126 who does this="h" for reasons I don't totally understand

😅 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.
image

You have my blessing to break or warn on this as you wish, lol.

@spences10
Copy link
Member

  • @spences10, whose use case is easily satisfied by other means such as this={"script"} (side note: it's pretty weird that we make this="input" behave exactly like an <input>, but we don't make this="script" behave exactly like a <script>

😅

After the discussion with @dummdidumm on #10130 I think I'll be dropping what you've mentioned here @Rich-Harris 👍

@Rich-Harris
Copy link
Member Author

It also adds needless complexity to the code base

Au contraire, the diff is mostly red: #11454

I can get behind not replicating behavior

It feels very weird to have a breaking change (<svelte:element this="input" bind:value> no longer works) but also keep some aspects of the existing behaviour (e.g. futzing around with namespaces in non-existent use cases). So either we should add back the weird behaviour (with all the attendant complexity) or just get rid of this strange wart. I vote the latter

@dummdidumm
Copy link
Member

dummdidumm commented May 3, 2024

Why is it strange that bind:value no longer works? You're using svelte:element, so this is very intuitive/to be expected that this stuff no longer works. Disallowing this="input" and saying "oh if you really need this" (which 90% of all people reaching for it do) "then you can do this={'input'}. That feels much weirder to me than not making bind:value work. It's also a breaking change which is not really necessary and will make more apps break compared to not readding the bind:value etc behavior. Just one more of those tiny breaking changes which add up, preventing more people from having a seamless upgrade.

Au contraire, the diff is mostly red: #11454

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.

@Rich-Harris
Copy link
Member Author

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.

and saying "oh if you really need this" (which 90% of all people reaching for it do)

Of the two people we've identified that are doing this, 0% say it's important that we keep this.

@dummdidumm
Copy link
Member

dummdidumm commented May 4, 2024

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 {"slot"} or use @html instead for no good reason.
Using static tag + element-specific binding is probably close to non-existent in practice.
That's why I advocate for the "remove special static handling but otherwise leave people alone".

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.

@baseballyama
Copy link
Member

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.
It would also have an good effect on code consistency of userland.

@Rich-Harris
Copy link
Member Author

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:

  • there's still a breaking change (behaviour of <svelte:element this="input"> has changed, but unless we want to go truly overboard then it will be a silent change)
  • we preserve a very stupid Svelte 4 bug (<svelte:element this="h{depth}"> results in <h>) that we absolutely should not waste time and energy fixing
  • it's storing up more work to do later — we have to remember to add the error in Svelte 6, instead of just ripping the bandaid off now and forgetting about it

@dummdidumm
Copy link
Member

I vote for removing the code around special-casing this-strings like in #11454 but only give a warning.

@Rich-Harris
Copy link
Member Author

That's what #11641 is, no?

@Rich-Harris
Copy link
Member Author

(note that it's a PR against #11454, not main — can change that if it's clearer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants