svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Property syntax using curly braces and quotes is inconsistent.

Open brunnerh opened this issue 3 years ago • 18 comments

Describe the bug

There is this special syntax prop="{value}" that does not cause string interpolation but instead just passes the value of the expression as is. This is inconsistent with how properties and string interpolation behave in all other cases.

As I see it, as soon as quotes are used, the value should be an interpolated string, no magic. Obviously, this would be a significant breaking change, so this is more of a suggestion for the future.

Reproduction

<Child value="42" /> <!-- string -->
<Child value={42} /> <!-- number -->
<Child value="{42}" /> <!-- (magically) number -->
<Child value="{42} " /> <!-- string -->

REPL

Logs

No response

System Info

REPL (Svelte Version 3.51.0)
Windows 10, 64bit
Chrome 106.0.5249.103

Severity

annoyance

brunnerh avatar Oct 10 '22 18:10 brunnerh

This dates back a long time ago (I think Svelte 1?) where people would generally use HTML syntax highlighting when editing Svelte components, so we encouraged people to use foo="{bar}" rather than foo={bar} so that their file would be highlighted correctly. I wouldn't be against changing this in Svelte 4.

Conduitry avatar Oct 10 '22 18:10 Conduitry

A counter argument to changing this is that you can do this today: class="{foo} some string"

dummdidumm avatar Oct 10 '22 18:10 dummdidumm

How is that a counter argument? This is just string interpolation in exactly the way I expect it to work. (Same as in element text content.)

brunnerh avatar Oct 10 '22 18:10 brunnerh

I'm sorry, forget my comment, didn't read thoroughly enough. Note to self: this would also need adjustment in prettier-plugin-svelte

dummdidumm avatar Oct 10 '22 19:10 dummdidumm

Given how big of a breaking change this is I suggest to first adjust prettier-plugin-svelte rules around this in a major that aligns with the Svelte 4 release to never have quotes in that scenario, and then see if we do it for Svelte 5.

dummdidumm avatar Feb 22 '23 11:02 dummdidumm

Either add some types or configure your prettier/linter to format or yell at you.

koodeau avatar Jun 09 '23 19:06 koodeau

If you’re passing the string value to a component property which expects a number then you’d get an error message immediately. I guess jsdoc can solve this problem by specifying property types if you’re not using typescript.

koodeau avatar Jun 22 '23 12:06 koodeau

Revisiting this, I still don't think we should change this behavior. It's arguably an edge case and AFAIK some people still use the prettier plugin's HTML strict mode due to editor limitations which means quotes are always applied. I'm in favor of kicking this can down the road further.

dummdidumm avatar Feb 26 '24 12:02 dummdidumm

Sure, it's just a minor thing anyway.

brunnerh avatar Feb 26 '24 12:02 brunnerh

Here's what I think we should do: in dev mode, print a warning if a component prop or element on[x] attribute is quoted and the value is a non-string. Something like this:

The foo="{bar}" prop will be coerced to a string in Svelte 6. If this isn't what you want, remove the quote marks

(Challenge: indicating where in the source code the offending prop was.)

If we do that, we can fix this weirdo behaviour in Svelte 6.

Rich-Harris avatar Apr 03 '24 21:04 Rich-Harris

Does Svelte still support this? The downside of disallowing x="{y}" syntax is that it breaks functionality in other frameworks. For example, HEEX templated in Phoenix/Elixir already use the exact syntax with curly braces, so, when I do something like this:

This is component.heex: (and not SVELTE)

<mycomponent customval={43}/>

Phoenix renders it as

index.html:

<mycomponent customval="43"/>

But, my component still works. With this change proposed, now I will have to end up doing something like:

<div data-customval="43">
   <mycomponent/>
</div>

And then have to fetch it inside my component.

dsignr avatar Jun 09 '24 11:06 dsignr

I don't understand what a different template language has to do with this change? Could you elaborate? What is mycomponent in your example? What is the connection to Svelte here?

dummdidumm avatar Jun 10 '24 08:06 dummdidumm

@trueadm when implementing this we need make sure that we have some way to know from the AST that the attribute was quoted. This is important for editor tooling to know. (I guess you need to adjust the AST in some way anyway, because the compiler also needs to know)

dummdidumm avatar Jun 10 '24 09:06 dummdidumm

@trueadm when implementing this we need make sure that we have some way to know from the AST that the attribute was quoted. This is important for editor tooling to know. (I guess you need to adjust the AST in some way anyway, because the compiler also needs to know)

How do you mean? Can you explain more please on why we need to change the AST?

trueadm avatar Jun 10 '24 11:06 trueadm

Because in Svelte 6 <Foo bar={baz} /> means Foo(.., { bar: baz } but <Foo bar="{baz}" /> means Foo(.., { bar: stringify(baz) }). So we need to differentiate it. But today that isn't possible solely by looking at the AST. You would have to do string checks on the original code to see whether the expression was quoted or not, which is not ideal.

dummdidumm avatar Jun 10 '24 11:06 dummdidumm

I don't understand what a different template language has to do with this change? Could you elaborate? What is mycomponent in your example? What is the connection to Svelte here?

I meant, there are other frameworks which depend on the syntax supporting the "" quoted syntax.

Assume I have a component like below. This is my menu.svelte:

<menu items={items}/>

Previously, this also used to work:

<menu items="{items}"/>

Now, my understanding is that it will no longer work post this particular issue being closed? As this quoted syntax support above allowed other frameworks outside of Svelte to support passing items from backend code which had a very similar syntax to Svelte. The one I quoted originally is from Phoenix/Elixir (reference).

dsignr avatar Jun 10 '24 18:06 dsignr

I'm afraid I'm not following. <menu items={items} /> doesn't do anything useful inside Svelte, because lowercase tags are treated as elements (and in fact there's already a <menu> element). So presumably that code is already being transformed into something that Svelte can deal with, in which case why couldn't the quotes be removed at the same time?

Rich-Harris avatar Jun 10 '24 19:06 Rich-Harris

Re AST changes: rather than adding a quoted boolean property or something, maybe we want to look towards Svelte 6:

export interface Attribute extends BaseNode {
  type: 'Attribute';
  name: string;
  value:
    | { type: 'boolean' }
    | { type: 'expression'; expression: Expression }
    | { type: 'text'; chunks: Array<Text | ExpressionTag> };
  metadata: {};
}

Then, if an attribute has a type: 'text' value with a single ExpressionTag chunk, we treat it as a type: 'expression' value and — if at runtime the value is a non-string value — warn. In Svelte 6, we just delete that logic.

Rich-Harris avatar Jun 10 '24 19:06 Rich-Harris

I'm looking at this more closely right now and it's a bit tough.

From a code completion (language tools) and formatting (prettier) angle:

  1. right now, code completion on DOM elements auto-adds quotes. That's probably fine for most of those, since they either expect string content (e.g. for class or style you most likely enter a static string) or are coerced to strings anyway. But not for all, which is where it gets hairy
  2. if prettier doesn't care about quotes anymore (i.e. leaves them in place, instead of always adding or removing them depending on the strict mode option), then the completion behavior in 1) will lead to a lot of wrong quotes down the line

For 1) we can look into completion not adding quotes automatically, which would at least help for IDEs that use the Svelte language server. For 2) we can tell Prettier to always remove quotes for Svelte 5.

Still, these measures likely don't catch all cases / reach all people.

Furthermore there's a much bigger problem I see: once Svelte 6 is out with the warning removed and the formatting rule adjusted to just leave quotes as-is, it will become much more likely that people accidentally add quotes and then are wondering why their code does not work. The problem, somewhat unique to Svelte, is that the way to pass the value as-is (foo={bar}) and stringified (foo="{bar}") is so closely related. Other frameworks don't have this problem:

  • Vue and Angular have special syntax for passing a variable as a prop where the syntax of the property name is different, not the value (Vue :foo="bar", Angular: [foo]="bar"), which is more visible. This also doesn't allow mixing strings and expressions.
  • JSX doesn't allow mixing strings with expressions, if you use quotes the { } characters inside are treated as literals, not as expression boundaries.

I also haven't seen anyone complain about the behavior from the angle of "I wanted to make this a string, but it's not, because the quotes are essentially ignored". So to summarize, this change removes a slight inconsistency but introduces a much more relevant and more likely source of error. This makes me hesitant to make the change this way.

Alternatives:

  • DOM elements continue to be ignorant of the quotes. To my knowledge there's no attribute that is either a string or something else, they all require one specific type of value. This reduces the blast radius of this, but keeps things inconsistent
  • Disallow quotes around a single expression completely to avoid the ambiguity. If you want the value stringified, do something like foo={String(bar)}
  • Mix of the first two options: ignore it for DOM elements, warn or error (on components only) that it's ambiguous
  • Keep it as is

dummdidumm avatar Jul 05 '24 12:07 dummdidumm

The warning is in place for components and custom elements now, moving this to 6.0 so we think about the next steps for that major when it comes to that.

dummdidumm avatar Jul 17 '24 21:07 dummdidumm

Thanks @Rich-Harris and @dummdidumm for taking a closer look!

dsignr avatar Jul 18 '24 07:07 dsignr

@dummdidumm I think the warning should be extended to elements, e.g. with the string conversion logic, event handlers in quotes do not make much sense:

<button onclick="{increment}">
	clicks: {count}
</button>

Or with certain properties like files in an input:

<input type="file" bind:files="{files}" />

brunnerh avatar Jul 18 '24 08:07 brunnerh

It's not added there because for onclick and bind:files there's only one possible kind of value, so it doesn't matter, whereas for attributes stringified or not makes a difference. But, we'll possible fade these out over time, too (warning, then at some point maybe even error) - depends on how good the other transitions works out.

dummdidumm avatar Jul 18 '24 13:07 dummdidumm

There's a solid chance I'm not following this well yet for how to mitigate / update for the change - but one case that is now throwing errors is when using pug via svelte-preprocess.

With this you need to quote some attributes to be able to use them (and can optionally control how they are encoded with the = or =! operator); e.g. -

<template lang="pug">

CustomComponent(propName='{ callFunction }')

CustomComponent(propName!='{ () => doSomething = true }')

</template>

Both of these syntax throw a warning cases like this (on a Component, but not on a standard element like a button).

https://github.com/sveltejs/svelte-preprocess/blob/main/docs/preprocessing.md#pug

Is there are way to disable the warning project-wide (or file wide, rather than multiple inline svelte-ignore attribute_quoted comments?

ClaytonFarr avatar Jul 21 '24 14:07 ClaytonFarr

Recently a warningFilter compiler option was added which could help:

compilerOptions: {
	warningFilter: ({ code }) => code != 'attribute_quoted',
},

Though the preprocessor probably needs to be adjusted if it currently always outputs the quotes 🤔

brunnerh avatar Jul 21 '24 15:07 brunnerh