sveltejs / svelte

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

Svelte 5: $state with computed properties + symbol (identifier) table #11358

Closed fcrozatier closed 4 months ago

fcrozatier commented 4 months ago

Describe the bug

... doesn't work.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE02PTWvDMAxA_4oQO6QQtntWB8bOPe1Y9eA5Smbm2CZSBiHkv484LfSkjyeepBV7H1iwua4Y7cjY4EfOWKMueS_kj4My1ihpntzeOYubfFYINg6GUIWwpUjqUhQFWcbvFMDAV0mq0_sT08nHAQwQujTmWbkjLLzMBCsCl-WzxLW09Xr4bmDgRdQqV70Nwod1x0X5hAkJCy08RRe8-wUD1cm0d-dxTgr8GtJQ6Y-Xx5aHdtvDdr_s_HY83FLEGsfU-d5zh41OM2-37R_7zjKHQQEAAA==

Logs

No response

System Info

preview

Severity

would be great

fcrozatier commented 4 months ago

In the case of the string it's not too much a problem as #11326 will allow to define these directly.

But in the case of a symbol it's more problematic.

Computed properties could be allowed for const which would make things easier for the compiler to track.

Motivation : this pattern would allow some interesting "private if you don't have the key" pattern

This probably means the compiler should build an identifier table. (Tangent : would be awesome to expose it for preprocessors)

brunnerh commented 4 months ago

If you need encapsulation, you can use a private field.

class MyClass {
    #state = $state(false);
    onclick() {
        console.log(this.#state);
    }
};
fcrozatier commented 4 months ago

But even with private fields it spills over to some amount

Capture d’écran 2024-04-28 à 18 13 05
brunnerh commented 4 months ago

That is not the same. TypeScript private qualified properties just try to enforce access/hide the property in the types but the property is not actually private at runtime.

Real private properties do not conflict and cannot be accessed from outside the class.

class A {
    #state = false;
    onclick() {
        console.log(this.#state);
    }
};
new A().onclick();

class B extends A {
    #state = true;
    onclick() {
        console.log(this.#state);
    }
};
new B().onclick();
fcrozatier commented 4 months ago

I didn't know the different private field / modifier behaviors in details, and I think you're correct in my case this is what I want to do. Thank you @brunnerh

Leaving this open for now while waiting for more feedback as it may still make sense in the bigger picture for Svelte to extend this js syntax / allow this pattern.

Plus being able to access the identifiers table after compilation would allow awesome smart behaviors for preprocessors. But maybe this is a separate discussion and should go in another issue?

dummdidumm commented 4 months ago

What we can do is to create a stable private identifier that doesn't clash with other identifiers and then use a computed getter/setter pair, so that the compiled result would look something like this:

class Foo {
 #unique_id_123 = ..;
 get [x]() { return get(this.#unique_id_123) }
 set [x]() { .. }
}
david-plugge commented 4 months ago

I also encountered an issue that seems similar:

<script lang="ts">
    class Test {
        #value = $state(1);

        get props() {
            const that = this;

            return {
                get value() {
                    return that.#value;
                }
            };
        }
    }

    const test = new Test();
</script>

<pre>{JSON.stringify(test.props, null, 2)}</pre>

The result is:

{
  "value": {
    "f": 0,
    "reactions": null,
    "v": 1,
    "version": 0,
    "inspect": {}
  }
}
7nik commented 4 months ago

@david-plugge, your issue is unrelated to this one, though the solution is the same: turn #value into setter + getter of #unique_id_123. Edit: your issue is #11476

trueadm commented 4 months ago

I spent this morning looking into seeing this was viable – the short story is that it's not because of the amount of complexity it adds to the compiler. If we were to allow computed public and private class property definitions, then that means we need to generate backing random private fields to store the state as @dummdidumm did above. That's fine and do-able for the class definitions as we can use the definition AST node as a key in our map to reference the random key we generate. However, this becomes very complicated on the usage sites – as this[foo.bar?.yar] now needs to reference the parent – and because of how computed properties work. this[foo.bar.yar] and this[computed()] can all reference the same thing under the hood – so it's actually impossible to statically guarantee they reference the same things – and we need this at a usage level to know to do $.proxy` on the assignment etc.

So closing this as it's likely not possible and definitely not worth the complexity doing it statically.