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

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

Closed
fcrozatier opened this issue Apr 28, 2024 · 9 comments
Closed
Assignees
Milestone

Comments

@fcrozatier
Copy link
Contributor

fcrozatier commented Apr 28, 2024

@fcrozatier
Copy link
Contributor Author

fcrozatier commented Apr 28, 2024

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)

@fcrozatier fcrozatier changed the title Svelte 5: $state with computed properties Svelte 5: $state with computed properties + symbol (identifier) table Apr 28, 2024
@brunnerh
Copy link
Member

brunnerh commented Apr 28, 2024

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

class MyClass {
	#state = $state(false);
	onclick() {
		console.log(this.#state);
	}
};

@fcrozatier
Copy link
Contributor Author

But even with private fields it spills over to some amount

Capture d’écran 2024-04-28 à 18 13 05

@brunnerh
Copy link
Member

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
Copy link
Contributor Author

fcrozatier commented Apr 28, 2024

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

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]() { .. }
}

@dummdidumm dummdidumm added this to the 5.0 milestone Apr 29, 2024
@david-plugge
Copy link

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

7nik commented May 5, 2024

@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 trueadm self-assigned this May 11, 2024
@trueadm
Copy link
Contributor

trueadm commented May 20, 2024

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.

@trueadm trueadm closed this as completed May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants