sveltejs / svelte

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

Using `this` alias breaks reactivity #11480

Closed TGlide closed 1 week ago

TGlide commented 1 week ago

Describe the bug

When using an alias for this in a Class with runes, setting a state using the alias breaks the signal.

Reproduction

REPL

Try to undo something on the 2nd example

Logs

No response

System Info

System:
    OS: macOS 13.4
    CPU: (8) arm64 Apple M2
    Memory: 86.95 MB / 24.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.6.2 - /usr/local/bin/node
    npm: 10.2.4 - /usr/local/bin/npm
    pnpm: 8.15.4 - /usr/local/bin/pnpm
    bun: 1.0.29 - ~/.bun/bin/bun
  Browsers:
    Safari: 16.5

Severity

annoyance

paoloricciuti commented 1 week ago

Btw this is only true for private fields REPL.

This is because when a private field is accessed with this the code get's converted by adding a .v at the end which actually set the value of the signal. By doing it on an instance you are tricking the compiler in not doing that.

image

while with public fields the set still goes through the setter which actually does a set on the signal.

paoloricciuti commented 1 week ago

Basically here we should check if node.object.type is ThisExpression but also if the name is any name to which this has been assigned to:

https://github.com/sveltejs/svelte/blob/34079a0ec57693cb419cf8ebffdb64f3ce69f24e/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js#L16-L34

dummdidumm commented 1 week ago

Is This the same as #11476 ? I think the easiest solution is to have a private getter/setter pair for each private state/derived backed by another private field. Bonus points for only doing that when we need to (i.e. when we detect this is assigned/passed to something)

paoloricciuti commented 1 week ago

Is This the same as #11476 ? I think the easiest solution is to have a private getter/setter pair for each private state/derived backed by another private field. Bonus points for only doing that when we need to (i.e. when we detect this is assigned/passed to something)

Yeah I think is the same. Since the problem is only there with private fields because public gets rewritten to private+get&set even if you return this you'll have no way to access them outside.

I like your idea...we should explore every method of the class to be sure this is not assigned to a class property right?

dummdidumm commented 1 week ago

I would add metadata.needs_private_getters to the class node and set that to true in the analysis phase when detecting a this expression that is not the first part of a member expression. Then in the code gen phase create those private getters/setters if that field is true

paoloricciuti commented 1 week ago

I would add metadata.needs_private_getters to the class node and set that to true in the analysis phase when detecting a this expression that is not the first part of a member expression. Then in the code gen phase create those private getters/setters if that field is true

Great I'll try to give it a shot tomorrow if I have time.