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

Using this alias breaks reactivity #11480

Closed
TGlide opened this issue May 5, 2024 · 6 comments · Fixed by #11487
Closed

Using this alias breaks reactivity #11480

TGlide opened this issue May 5, 2024 · 6 comments · Fixed by #11487
Assignees

Comments

@TGlide
Copy link
Member

TGlide commented May 5, 2024

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

paoloricciuti commented May 5, 2024

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

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:

if (node.object.type === 'ThisExpression') {
// rewrite `this.#foo` as `this.#foo.v` inside a constructor
if (node.property.type === 'PrivateIdentifier') {
const field = state.private_state.get(node.property.name);
if (field) {
return state.in_constructor ? b.member(node, b.id('v')) : b.call('$.get', node);
}
}
// rewrite `this.foo` as `this.#foo.v` inside a constructor
if (node.property.type === 'Identifier' && !node.computed) {
const field = state.public_state.get(node.property.name);
if (field && state.in_constructor) {
return b.member(b.member(b.this, field.id), b.id('v'));
}
}
}
next();

@dummdidumm
Copy link
Member

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

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

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

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.

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

Successfully merging a pull request may close this issue.

3 participants