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

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

Closed
4 tasks done
AlbertMarashi opened this issue Aug 14, 2024 · 7 comments

Comments

@AlbertMarashi
Copy link

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

  • I'm using eslint-plugin-svelte. (*.svelte file linting does not work with the parser alone. You should also use eslint-plugin-svelte with it.)
  • I'm sure the problem is a parser problem. (If you are not sure, search for the issue in eslint-plugin-svelte repo and open the issue in eslint-plugin-svelte repo if there is no solution.
  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.

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 AlbertMarashi changed the title Mustache expressions should be wrapped in template literals `(${bar});` for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals ``(${bar}); for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals ``(${bar}); for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals ``(${bar});`` for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals ``(${bar});`` for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals (\${bar}\); for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals (\${bar}\); for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals ``(\${bar}\);`` for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals ``(\${bar}\);`` for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals `` (\${bar}\); `` for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals `` (\${bar}\); `` for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals ` (\${bar}\); ` for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals ` (\${bar}\); ` for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi AlbertMarashi changed the title Mustache expressions should be wrapped in template literals (${bar}); for consistency with typescript eslint rules Mustache expressions should be wrapped in template literals for consistency with typescript eslint rules Aug 14, 2024
@AlbertMarashi
Copy link
Author

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

@AlbertMarashi
Copy link
Author

Linking for reference sveltejs/language-tools#2465

@ota-meshi
Copy link
Member

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.

sveltejs/eslint-plugin-svelte#747

@ota-meshi ota-meshi closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2024
@AlbertMarashi
Copy link
Author

@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
Copy link
Member

ota-meshi commented Sep 13, 2024

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
Copy link
Author

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
Copy link
Member

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.

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

No branches or pull requests

2 participants