sveltejs / svelte

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

Svelte 5: Nonstate reference warning is too aggressive #9684

Closed Rich-Harris closed 5 days ago

Rich-Harris commented 8 months ago

Describe the bug

9669 introduced a warning for mutated nonstate that is referenced in the template. It shouldn't apply in cases like this, because the compiler should assume that obj.count could be an accessor:

<script>
    let count = $state(0);

    let obj = {
        get count() {
            return count;
        },
        set count(v) {
            count = v;
        }
    }

    function increment() {
        obj.count += 1;
    }
</script>

<button on:click={increment}>
    clicks: {obj.count}
</button>

In other words it should only apply to identifiers that aren't part of a MemberExpression, and only when the value is reassigned rather than simply mutated.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE01QSw7CIBC9CiEuNBo_W9qaeA7rosWpQduhgaGJIdxdWmx1NXlv3ofB80a1YLm4eo5VB1zwS9_zHad3PwI7QEsQsdXOyJHJrTSqp3OJJbVATGqHxAq2slQRrI-bLG6-O10_48aPsKTHLF5vZqokA-QMJj5LZNilaRf98GeY64ZZPY6QKhuHkpRGplAa6OC_Kj5ln7zbgp2yrys__K7BvHZE0a1RyFbJV-GXnDCdO7FWML-ETRHJdo6f1Om7ahTcuSDjINzCB3wP_0BfAQAA

Logs

No response

System Info

next

Severity

annoyance

sockmaster27 commented 8 months ago

I'm also getting this warning on a TypeScript enum:

<script lang="ts">
  enum Which {
    First,
    Second,
  }
  let which = $state(Which.First);
</script>

<button onclick={(): void => {
        which = Which.Second;
    }}>
    {which}
</button>
warning  Which is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates.(non-state-reference)  svelte/valid-compile

I'm only getting it from eslint and not Svelte itself though.

thorsby commented 7 months ago

I'm seeing the same warning with the below simplified example. It's used to wrap an external library as a component.

<script>
let ref;
let grid;
grid = new Grid(ref);
</script>

<div bind:this={ref} />

The warning is both in the compiler and Svelte extension for VSC. ref is updated, but is not declared with $state(...). Changing its value will not correctly trigger updates.

System info svelte@5.0.0-next.27

gregmulvaney commented 7 months ago

Way to aggressive. Dom refs don't need to be reactive. Even the listed examples in Svelte 5 docs disagree with compiler and eslint warnings.

https://component-party.dev/?f=svelte4,svelte5#dom-ref

trueadm commented 5 months ago

This has been fixed.

eunukasiko commented 5 months ago

Still seeing this for enum which is declared in <script lang="ts" context="module"> block

quinnvaughn commented 2 weeks ago

It has not been fixed for DOM elements being binded @trueadm

Rich-Harris commented 2 weeks ago

Can you provide a repro in the playground?

cmendoza-cs commented 1 week ago

Experiencing this issue as well, the repro looks like this, though I don't think the playground shows those kinds of link warnings (or does it?): https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE0VPTWvDMAz9K0Ls0LJAumuaFHbYP9itGTRxlEbMkUMsrwzj_z6cQHfU-9J7EUe25LG6RpRuJqzwfVmwQP1d8uF_yCphgd6F1WSk9mblRS-ttGpJwbggCg28eO2UDqfjuZXMjUGMshNgMSvNJHo4QsxMq7vntYG3LNa0O3Jar_Jhz1CW8DmxB8tCcCf1oBPBo1uF5Q5DIFC3QbeeZah0Yn-Dnqx7tFKX_w2l7oOqE3BiLJvvJj7bJHham7i9TdumTecriFvJlPP2jAsWOLuBR6YBK10Dpa_0B4kJDiNAAQAA

Anyway, this is what it looks like on my editor image

paoloricciuti commented 1 week ago

Experiencing this issue as well, the repro looks like this, though I don't think the playground shows those kinds of link warnings (or does it?): https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE0VPTWvDMAz9K0Ls0LJAumuaFHbYP9itGTRxlEbMkUMsrwzj_z6cQHfU-9J7EUe25LG6RpRuJqzwfVmwQP1d8uF_yCphgd6F1WSk9mblRS-ttGpJwbggCg28eO2UDqfjuZXMjUGMshNgMSvNJHo4QsxMq7vntYG3LNa0O3Jar_Jhz1CW8DmxB8tCcCf1oBPBo1uF5Q5DIFC3QbeeZah0Yn-Dnqx7tFKX_w2l7oOqE3BiLJvvJj7bJHham7i9TdumTecriFvJlPP2jAsWOLuBR6YBK10Dpa_0B4kJDiNAAQAA

Anyway, this is what it looks like on my editor image

It should...can you check which version of svelte you are using locally?

cmendoza-cs commented 1 week ago

It should...can you check which version of svelte you are using locally?

I currently have 5.0.0-next.210

7nik commented 1 week ago

Experiencing this issue as well, the repro looks like this, though I don't think the playground shows those kinds of link warnings (or does it?): https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE0VPTWvDMAz9K0Ls0LJAumuaFHbYP9itGTRxlEbMkUMsrwzj_z6cQHfU-9J7EUe25LG6RpRuJqzwfVmwQP1d8uF_yCphgd6F1WSk9mblRS-ttGpJwbggCg28eO2UDqfjuZXMjUGMshNgMSvNJHo4QsxMq7vntYG3LNa0O3Jar_Jhz1CW8DmxB8tCcCf1oBPBo1uF5Q5DIFC3QbeeZah0Yn-Dnqx7tFKX_w2l7oOqE3BiLJvvJj7bJHham7i9TdumTecriFvJlPP2jAsWOLuBR6YBK10Dpa_0B4kJDiNAAQAA

Anyway, this is what it looks like on my editor image

The REPL can be oversimplified. Where and how do you use btnEl?

paoloricciuti commented 1 week ago

Oh i just realized: it will only trigger if you have the bound element in an if or each or key basically some block that could make the element change

example

cmendoza-cs commented 1 week ago

Oh i just realized: it will only trigger if you have the bound element in an if or each or key basically some block that could make the element change

Oh wow, I was just trying to figure out why some of my bound variables don't exhibit that issue. I just tested and yeah that does seem to explain it.

Rich-Harris commented 5 days ago

I'll close this as I think we've covered the various cases — we can reopen with examples if not