sveltejs / svelte-eslint-parser

Svelte parser for ESLint
https://sveltejs.github.io/svelte-eslint-parser/
MIT License
95 stars 21 forks source link

Mustache expressions should be wrapped in template literals for consistency with typescript eslint rules #558

Closed AlbertMarashi closed 2 months ago

AlbertMarashi commented 3 months ago

Before You File a Bug Report Please Confirm You Have Done The Following...

What version of ESLint are you using?

0.41.0

What version of eslint-plugin-svelte and svelte-eslint-parser are you using?

What did you do?

The following is a major footgun and surface area for bugs.

<script lang="ts">

let obj = { 
  a: bool
}
</script>
<a href="/foo/{obj}">...</a>

Will result in

<a href="/foo/[object Object]">..</a>

Even in situations such as

Hello { obj }

We get

Hello [object Object]

There's exactly zero times that I've desired this behavior & almost always a surface area for bugs when refactoring props, types and etc.

This can quite easily occur throughout projects during refactoring or even just developer oversight. We should prevent this from occurring.

I view this as a quite serious typing issue, with no easy apparent remedies.

What did you expect to happen?

What actually happened?

<script lang="ts">
    const array = [1, 2, 3]
</script>
{#each array as e}
    {e}
{/each}

Gets turned into:


    const array = [1, 2, 3]
;function $_render1(){        

Array.from(array).forEach((e) => {
    (ee);
});
}

When it should be

    const array = [1, 2, 3]
;function $_render1(){        

Array.from(array).forEach((e) => {
    (`${ee}`);
});
}

Link to GitHub Repo with Minimal Reproducible Example

NA

Additional comments

No response

AlbertMarashi commented 3 months ago

Whoops sorry, couldn't figure out how to escape that code

AlbertMarashi commented 3 months ago

Linking for reference https://github.com/sveltejs/language-tools/issues/2465

ota-meshi commented 2 months ago

There's no template literal syntax there, so I wouldn't parse it as a template literal. I think it's reasonable to provide new rule that you're already working with.

https://github.com/sveltejs/eslint-plugin-svelte/issues/747

AlbertMarashi commented 2 months ago

@ota-meshi to clarify the reasoning for this is that mustache expressions are treated as strings to render in the DOM.

Therefore it makes sense to wrap them in template expressions for the purposes that typescript can understand that the return value is going to be a string.

This is logically the approach that would make sense in the virtual script code, I think the entire render code should be effectively viewed as essentially one big template literal

ota-meshi commented 2 months ago

Are you talking about type information and not the AST? Does getting type information using template literals change anything?

I think all ESLint rules that parse template literals will report incorrectly if we assign the AST node of a template literal to a part that is not a template literal.

AlbertMarashi commented 2 months ago

Are you talking about type information and not the AST? Does getting type information using template literals change anything?

I think all ESLint rules that parse template literals will report incorrectly if we assign the AST node of a template literal to a part that is not a template literal.

I'm specifically talking about the semantics of virtual code.

Array.from(array).forEach((e) => {
    (ee);
});

has different semantics to

Array.from(array).forEach((e) => {
    (`${ee}`);
});

In the former case, eslint doesn't care that ee isn't a stringifiable type, but in the latter it does (and should)

Ideally, we want to prevent situations where the following is treated as valid code

<a href="/foo/[object Object]">..</a>
ota-meshi commented 2 months ago

Virtual code is a mechanism for obtaining type information in TypeScript. It is implemented to revert to the original code when converting it to AST, so using template literals in virtual code will not improve the behavior of that rule.