sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.21k stars 4.09k forks source link

Property syntax using curly braces and quotes is inconsistent. #7925

Open brunnerh opened 1 year ago

brunnerh commented 1 year ago

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

Conduitry commented 1 year ago

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.

dummdidumm commented 1 year ago

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

brunnerh commented 1 year ago

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.)

dummdidumm commented 1 year ago

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

dummdidumm commented 1 year ago

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.

koodeau commented 1 year ago

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

koodeau commented 1 year ago

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.

dummdidumm commented 6 months ago

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.

brunnerh commented 6 months ago

Sure, it's just a minor thing anyway.

Rich-Harris commented 5 months ago

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.

dsignr commented 3 months ago

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.

dummdidumm commented 3 months ago

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 commented 3 months ago

@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)

trueadm commented 3 months ago

@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?

dummdidumm commented 3 months ago

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.

dsignr commented 3 months ago

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).

Rich-Harris commented 3 months ago

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 commented 3 months ago

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.

dummdidumm commented 2 months ago

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:

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:

dummdidumm commented 1 month ago

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.

dsignr commented 1 month ago

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

brunnerh commented 1 month ago

@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}" />
dummdidumm commented 1 month ago

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.

ClaytonFarr commented 1 month ago

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?

brunnerh commented 1 month ago

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 🤔