svelte/@typescript-eslint/no-unnecessary-condition possible false positive

[olafurw]

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

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

8.39.0

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

2.28.0

What did you do?

https://typescript-eslint.io/play/#ts=5.0.4&sourceType=module&code=KYDwDg9gTgLgBAG2PAFlYAzAXHAzjKASwDsBzOAHzgFdiATTE4OuAXhvseOYG4AoPgGMIxfDSgI2cNJjgB+OXADkKGDDBYA9JtABDALZgkAOmH6l-IA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0yHJgBNK+SpPRRE0aB2iRwYAL4gtQA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

I'm showing here in eslint playground but the same thing happens in a .svelte file, with both svelte/@typescript-eslint/no-unnecessary-condition and @typescript-eslint/no-unnecessary-condition

<script>
export let href: string | undefined = undefined;

const url = href ?? 'http://example.com';
</script>

It will tell me that the href condition check is unnecessary since it will always be null or undefined.
But it's an export variable, so it might get some value from whoever is using the property, I've just defaulted it to undefined if someone doesn't send it in.

What did you expect to happen?

No warning/error

What actually happened?

Warning: Unnecessary conditional, left-hand side of ??operator is alwaysnullorundefined. 3:13 - 3:17

Link to GitHub Repo with Minimal Reproducible Example

https://typescript-eslint.io/play/#ts=5.0.4&sourceType=module&code=KYDwDg9gTgLgBAG2PAFlYAzAXHAzjKASwDsBzOAHzgFdiATTE4OuAXhvseOYG4AoPgGMIxfDSgI2cNJjgB+OXADkKGDDBYA9JtABDALZgkAOmH6l-IA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0yHJgBNK+SpPRRE0aB2iRwYAL4gtQA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Additional comments

No response

[ota-meshi]

Shouldn't you write the following to make the url variable reactive?

<script lang="ts">
export let href: string | undefined = undefined;

$: url = href ?? 'http://example.com';
</script>

I think that linting will work well if you write it as above.

[ota-meshi]

https://eslint-online-playground.netlify.app/#eNqFU9tu2zAM/RVNGJBtqJN39zag3bCXbcAw7GUqCkNmErUypVFykCDLv5ey7OaCJIVfZJ1D8vCQWstAevJ9deca7xAwjsMCbARZSoVXQZPxUdgKZ9dKxqDkjUJYekd8CVHMCaalCJEMzsR/0WINU4NQi+vt+VKhwvelaMnydYoQt7diNI/Rl5MJLKvGWxhr14yYeTXJJW/kBZfTDqdmNn4KDlnPWqEQSjLTGwv000fjkCWVokMSlpTo+KO19m4O+rkDI7WQ8M1FjjeobVtDgv5yBGklHxRuuGByYtATA1fk+sHxj3WzD0p+A2vdOyU/XjJ3DMEajKRZHTMbV7dMzNYE7pMlqQjLCFiHVIj/+JM5qiTgLhrGoFaSdWXQ23ZmsMwDmJzlfI4rD9mqIufc5RcE/1qThlIkXqGTGfyneKYqPnTJfEXh1cTOwlQAdFP9AQp8WbJXtooQuGpfPriWNPzmlAnMPb+C3CxVX3kyX1LXKUPXuOwXil3uBiDEMNdfzsV7Q6V4fKwNYdVAT/DknkDHUoz2VmDUFUpjVNEtgMjUsPW2b0DFKUvIlT/xFFkeS02n5XDU29smHbMf6cuepN6O+JvBbOEg40TZ3Y4PM2esT1oMSQ+Ih2PZTbFmA9+QKDZD2LD0xJPafypHwtEVLSJoCKGiVcHW1ybpSHG8ukSua5+fUu9Bt0357fhKP1czOHirNSzuwaedRG3eFjDoP1i9k/zBxO5NnAjLnLNgn6AYxnaUehbcStsn9GZt5OYFIQXoNQ==

[olafurw]

<script lang="ts">
export let href: string | undefined = undefined;

const url = href ?? 'http://example.com';
</script>

<a href={url}>hello</a>

It doesn't always have to be reactive, it's just a parameter coming from someone that created the component, it's not going to change unless someone changes the url from the outside, which in that case the component will be redrawn.

Unless I'm wildly misunderstanding Svelte.

[ota-meshi]

Check those two REPLs. Your code is fixed with initial values.

https://svelte.dev/repl/9eead0107cbd4f32b626a634fe8af2dd?version=3.59.1

https://svelte.dev/repl/d530530e12394d59b52623b5b4366266?version=3.59.1

[olafurw]

Sure, again you're talking about reactivity, but why would it give me a warning telling me a value is always "null" or always "false" when I can do this. It's just not correct since it could be initialized with other values.

https://svelte.dev/repl/26022347d03b472190c9581b0640d901?version=3.59.1

[ota-meshi]

Please consider opening a pull request if you'd like to fix it.

[olafurw]

Thanks for looking into it, I'll see if I have time for a PR later on.

For anyone reading this, the workaround works and is fine.

[baseballyama]

I think instead of modifying this rule, it's better to create like svelte/prefer-reactive-label rule.
Because as @ota-meshi mentioned that if a parent component passes a prop AFTER mounting this component, it will be a bug.
So to avoid this situation, using a reactive statement is safer.

<script>
  export let href: string | undefined = undefined;
  const url = href ?? 'http://example.com';
// ^ Use reactive statement `$` instead of const / let / var.
</script>

[ota-meshi]

I like that new rule you proposed.

However, I think @olafurw intentionally does not use reactive variables.
#476 (comment)
I still don't understand why.

[baseballyama]

I think this makes a chance to introduce a bug, so in terms of best practice, it's better to use $ instead of const / let / var.
So personally I think this official plugin doesn't need to handle this issue.

And to handle this issue, again we need to implement svelte/@typescript-eslint/no-unnecessary-condition for this niche case?

[ota-meshi]

And to handle this issue, again we need to implement svelte/@typescript-eslint/no-unnecessary-condition for this niche case?

In my opinion, to fix the problem we need to fix the parser. I believe that removing the initial expression when parsing with typescript and reverting the AST will give we the correct one.
But I think it's very low priority and I'm not going to spend time fixing it unless someone makes a pull request.

[baseballyama]

I'm not sure about concrete cases but I'm afraid that the parser removing a default value will make a different issue.

[ota-meshi]

Yeah. I think we need a lot of tests. For example, we might want to process only exports with type annotations, as type inference doesn't work.

[mpiorowski]

I think i might have a similar problem, but it is not fixed by reactivity.

https://github.com/mpiorowski/rusve/blob/78bedb5dc4e491a53149a5476d8d711fa08347a7/client/src/lib/form/Button.svelte#L6

<script lang="ts">
    import LoadingComponent from "$lib/components/LoadingComponent.svelte";

    export let type: "button" | "submit" = "submit";
    export let loading = false;
    export let variant: "primary" | "secondary" | "error" = "primary";
    export let form: string | undefined = undefined;
    export let href = "";

    $: className =
        "w-full h-10 flex flex-row gap-3 justify-center items-center rounded px-4 shadow-md font-normal text-base text-white transition";
    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
    if (variant === "primary") {
        className += " bg-secondary-700 hover:bg-secondary-800";
    } else if (variant === "secondary") {
        className += " bg-primary-800 hover:bg-primary-900";
    } else if (variant === "error") {
        className += " bg-error-600";
    }
</script>

Even when i made the className a reactiv, it still see variant AS "primary" and ONLY "primary" :D

[ota-meshi]

I think you should write:

$: className =
    "w-full h-10 flex flex-row gap-3 justify-center items-center rounded px-4 shadow-md font-normal text-base text-white transition"
    + (
      variant === "primary"
      ? " bg-secondary-700 hover:bg-secondary-800"
      : variant === "secondary"
      ? " bg-primary-800 hover:bg-primary-900"
      : " bg-error-600"
    );

Also, I think overwriting the className of a reactive variable, contains an unintended error.
(Refer: https://sveltejs.github.io/eslint-plugin-svelte/rules/no-reactive-reassign/)

[mpiorowski]

@ota-meshi but there problem here is not className at all, check out this example

    export let variant: "primary" | "secondary" | "error" = "primary";

    if (variant === "primary") {
        console.log("primary");
    } else if (variant === "secondary") {
        console.log("secondary");
    } else if (variant === "error") {
        console.log("error");
    }

And the warning is here also.

[ota-meshi]

Yeah. It has already been reported this issue.

[ptrxyz]

consider this:

<script lang="ts">
  export let externalProp: {id: string} | undefined = undefined

  let internalState = externalProp;   // let's make a copy to work with inside the component

  onMount(() => {
    if (internalState) doSomething()   // according to eslint, this check is unnecessary.
                                       // However it is not since the component could be mounted 
                                       // with an undefined externalProp.
  })
</script>

<script lang="ts">
  type T = {id: string}

  export let externalProp: T | undefined = undefined
  let internalState: T | undefined = externalProp;   // let's make a copy to work with inside the component
                                                     // and explicitly type it

  onMount(() => {
    if (internalState) doSomething()   // this is now fine!
  })
</script>