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

Svelte typecasting where it shouldn't #657

Closed
alindsay55661 opened this issue Jun 21, 2017 · 17 comments
Closed

Svelte typecasting where it shouldn't #657

alindsay55661 opened this issue Jun 21, 2017 · 17 comments
Labels

Comments

@alindsay55661
Copy link

alindsay55661 commented Jun 21, 2017

Svelte is incorrectly casting numeric strings to numbers when assigned to component properties. This results in strange truthy behavior:

"1" is truthy
"true" is truthy
"false" is truthy

"0" is falsey
"0" is falsey

All of these strings should be truthy, but the number (and html entity for 0) are falsey.

https://svelte.technology/repl?version=1.22.5&gist=b75e1acf7ee81e6fb2b6d0ae0e8349d5

@Conduitry
Copy link
Member

Interesting. In one sense it does sound good for <Component value="0"/> to pass in the string "0" instead of the number 0. With that setup, you'd have to do something like <Component value="{{0}}"/> to pass the actual number 0.

However, I'm worried about that causing more confusion than it's worth. The only time something like this would come up I think is when you have literal values in the template. (If you had <Component value="{{someExpression}}"/> the value and type of someExpression would be passed through unchanged, which makes sense and doesn't need to change.)

What I'd suggest here for passing in the actual string "0" is to do <Component value="{{'0'}}"/>. It's a little long-winded, but I'd consider this a side-effect of making the language more usable.

@Conduitry
Copy link
Member

Conduitry commented Jun 21, 2017

Oh, I just noticed the part about <Component value="false"/> passing in the string "false" rather than the boolean false. Hm. If you just have <Component value/> then that passes in the boolean true, but you'd currently have to do <Component value="{{false}}"/> to pass in false. I'm actually not sure anymore what's the nicest way to make this a bit more consistent.

edit: My feeling is that the only ways to make this nicer would have to be considered breaking changes. I don't feel like there's a bug here really, just some undocumented nuances. The way it's implemented now, only things that look like numbers (i.e., !isNaN("attribute value")) are interpreted as non-string values - the rest are considered strings.

@PaulBGD
Copy link
Member

PaulBGD commented Jun 22, 2017

I'm not sure how to handle this. Do we convert "true"/"false" to true/false or do we pass everything as a string?

@Rich-Harris
Copy link
Member

This is entirely my bad — I thought it would be convenient to coerce numbers but didn't think about false/true etc, or edge cases where you want the string. In retrospect, that was probably a bad decision.

Since there's a relatively easy workaround I think we're probably okay leaving the current behaviour for now, and thinking about what we'd ideally like to happen for v2. I think the least surprising thing would be if everything was passed down as strings.

Since it's kind of ugly to write something='{{false}}' I wonder if we should seriously consider moving away from the {{ }} delimiters — they turn people away (particularly people who associate them with string-based logic-less templating systems of yore) and are probably overkill. We could take a leaf out of the JSX book and use single curlies:

<Component value={false}/>

<!-- maybe still treat quoted single tags as expressions,
     for the sake of syntax highlighters etc -->
<Component value={foo || bar}/>   <!-- colours are weird -->
<Component value='{foo || bar}'/> <!-- colours are fine -->

Realise that's a separate discussion (for which we should probably have a separate issue), but interested in immediate reactions.

@Conduitry
Copy link
Member

I like the idea of using a different syntax when you want to pass in anything other than a static string. It takes care of this problem, and (depending on the syntax that's settled upon) is a little easier to type. I don't know that I'd worry too much about how syntax current coloring treats the syntax we use. What I'm a little worried about is whether a syntax that looks like JSX but isn't would be much less upsetting than syntax that looks like mustache but isn't.

@Rich-Harris Rich-Harris added this to the 2.x milestone Jun 25, 2017
@Conduitry
Copy link
Member

Something else worthwhile might be to rethink what the tags for if/each/etc look like. {{#if}} and {{#each}} pairing with {{/if}} and {{/each}} makes sense but {{else}} and {{elseif}} and {{yield}} having no special character before the keyword to distinguish them from regular tags is a bit confusing to me. I haven't put a lot of thought into what another syntax might look like though.

Replacing {{yield}} with <:Yield/> might make sense, now that there's a precedent for special :-prefixed elements but I don't have a good suggestion for if/each.

@vp2177
Copy link
Contributor

vp2177 commented Jun 27, 2017

{{else}} is Handlebars syntax, it works because else is a keyword and can't be an identifier anyway. Same applies to {{yield}}.

@kbrsh
Copy link

kbrsh commented Aug 3, 2017

@Rich-Harris Both Vue and Moon have workarounds for this that I think can be applied to Svelte. All properties as passed as strings, but when using a special syntax (v-bind or m-literal), you can have a literal value. For Svelte, that might look something like:

<Component value="someString"/> <!-- passes a string "someString" -->
<Component literal:value="item"/> <!-- passes the value of "item" -->
<Component literal:value="true"/> <!-- passes the boolean true -->
<Component literal:value="0"/> <!-- passes the number 0 -->

Basically, items in the literal directive are treated as if they were inside of a mustache expression, and items without it are treated as strings. Optionally, attributes can include mustache templates as well, but not when using the literal directive.

@vp2177
Copy link
Contributor

vp2177 commented Aug 4, 2017

@kingpixil
What is really nice about Svelte's current syntax is that it is valid HTML. Keeps editors happy.
This would not be true for <element attr={true}>. While Vue & co. work around that, I really appreciate the elegance of Angular's syntax.

In a way it is the opposite of Svelte's: literal string attributes become the special case. It is elegant because the attribute value is simply always a JavaScript expression.
Works nicely for numbers <input value='5'>, booleans <Toggle on='true'>, and variables. Strings would be less elegant (<element attr='"string literal"'>), but in real use the string would mostly come from a variable or constant anyway.

@alindsay55661
Copy link
Author

but in real use the string would mostly come from a variable or constant anyway.

This is debatable. True for data bound values but often data binding is overkill. Many components use configuration that is rather static.

<Component theme="light" size="small" />

@vp2177
Copy link
Contributor

vp2177 commented Aug 4, 2017

True, although there's nothing wrong with <Component theme='"light"'> or <Component theme='::THEME'> (one-way binding to a constant).

One thing that's a lot nicer with Svelte's syntax currently is interpolation in attributes: <div style='transform: rotate({{b}}deg)'> vs Angular's <div style='"transform: rotate(" + b + "deg)"'>

@PaulBGD
Copy link
Member

PaulBGD commented Aug 19, 2017

I don't know if this is related, but one issue that annoys me is how Svelte handles undefined. Let's say I want a CustomButton that looks like this:

<button :id></button>

but when I use the CustomButton without an id, it passes undefined as an id which it then gets rendered as

<button id="undefined" svelte-xx></button>

which I think is unexpected behavior.. is this a different issue or related?

@vp2177
Copy link
Contributor

vp2177 commented Aug 19, 2017

@PaulBGD That is native DOM behaviour:

var button = document.createElement('button')
button.setAttribute('id', undefined)

This will also result in a <button id="undefined"> element.

@PaulBGD
Copy link
Member

PaulBGD commented Aug 20, 2017

Yes but it feels bad with binding, because there's no way to pass a value that represents no value (null maybe?)

@vp2177
Copy link
Contributor

vp2177 commented Aug 20, 2017

@PaulBGD I think #659 is related

@ricardobeat
Copy link

Second @PaulBGD, this is unnecessarily bloating the output. I don't see a realistic use case where you actually want the attribute value to be the string undefined. It should be possible to output if (id) button.id = id; and a ternary expression for SSR?

@Rich-Harris
Copy link
Member

Fixed, finally, in v2.0.0. Thanks!

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

No branches or pull requests

7 participants