Skip to content

Svelte 5 Exporting $state compile problem #14316

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

Open
jdgamble555 opened this issue Nov 15, 2024 · 17 comments · May be fixed by #15589
Open

Svelte 5 Exporting $state compile problem #14316

jdgamble555 opened this issue Nov 15, 2024 · 17 comments · May be fixed by #15589

Comments

@jdgamble555
Copy link

Describe the bug

If I want to export a shared state, the normal way is to do like this:

Option 1

// rune.svelte.ts

export const rune = <T>(initialValue: T) => {

    let _rune = $state(initialValue);

    return {
        get value() {
            return _rune;
        },
        set value(v: T) {
            _rune = v;
        }
    };
};

Option 2

This also works:

// rune.svelte.ts

export const rune = <T>(initialValue: T) => {
    const _rune = $state({ value: initialValue });
    return _rune;
};

Option 3

However, if I simplify option 2, I get a compile error:

export const rune = <T>(initialValue: T) => $state({ value: initialValue });

// OR

export const rune = <T>(initialValue: T) => {
  return $state({ value: initialValue });
};

I get the error:

$state(...)` can only be used as a variable declaration initializer or a class field

I would expect Option 3 to compile the exact same way as option 2 !?

Reproduction

REPL

Logs

Error message above.

System Info

Svelte 5.2.0

Severity

annoyance

@dummdidumm
Copy link
Member

This is somewhat on purpose - we want to restrict the places where you can use runes to not make it look like it's something that it isn't. We're open to loosen that in the future if more use cases come up / we're confident that the rules of runes are well understood enough.

@webJose
Copy link
Contributor

webJose commented Nov 15, 2024

Can this be made to work, @dummdidumm ? It is my understanding that $state is more an L-value than what it looks like (R-value).

@trueadm
Copy link
Contributor

trueadm commented Nov 15, 2024

What we could do is permit return $state() but add a runtime check in DEV that checks if the object is actually a proxy and throw an error/warning if not the case.

@Leonidaz
Copy link

I also found certain restrictions are overly strict, e.g.

Svelte compiler doesn't allow this for existing objects:

	const obj = {};

	obj.count = $state(0);

So, the workaround would have to be this but it's pretty cumbersome:

	const obj = {};
	let count = $state(0);
	Object.defineProperty(obj, 'count', {
		get: function() { return count },
		set: function(v) { count = v},
		enumerable: true,
	});

This works though since obj becomes a proxy but the whole object already has to be a state:

	const obj = $state({});

	obj.count = 0;

Incidentally, using Object.defineProperty on a proxified / state wrapped object doesn't work, in case you want to add a custom getter or setter, and results in an error:

state_descriptors_fixed Property descriptors defined on `$state` objects must contain `value` and always be `enumerable`, `configurable` and `writable`.

if writable: true is added, the JS blows up as you can't specify both writable and a setter: Invalid property descriptor. Cannot both specify accessors and a value or writable attribute

	const obj = $state({});
	let count = $state(0);
	Object.defineProperty(obj, 'count', {
		get: function() { return count },
		set: function(v) { count = v},
		enumerable: true,
                configurable: true,
	});

@trueadm
Copy link
Contributor

trueadm commented Nov 15, 2024

Just make the original object state, then you don’t need to create state to assign to it.

@Leonidaz
Copy link

Just make the original object state, then you don’t need to create state to assign to it.

right, i have it outlined in my comment above. just saying that if there is a non-state object, it's cumbersome to add properties to it vs just a simple assignment. And, for a state object defining custom getters and setters for a property doesn't work.

@trueadm
Copy link
Contributor

trueadm commented Nov 15, 2024

just saying that if there is a non-state object, it's cumbersome to add properties to it vs just a simple assignment.

You're trying to add a reactive state of 0, that simply won't work. The value is a primitive, so it won't be reactive as there's no way for the runtime to capture the signal without encapsulation. Maybe we can loosen rules around this for proxied state though, as that can encapsulate reactivity on assignment.

And, for a state object defining custom getters and setters for a property doesn't work.

Please can you create a separate issue for that, seems like a bug.

@Leonidaz
Copy link

just saying that if there is a non-state object, it's cumbersome to add properties to it vs just a simple assignment.

You're trying to add a reactive state of 0, that simply won't work. The value is a primitive, so it won't be reactive as there's no way for the runtime to capture the signal without encapsulation. Maybe we can loosen rules around this for proxied state though, as that can encapsulate reactivity on assignment.

Just for clarity, what I was aiming for is for svelte to perform a code transformation when assigning state directly.

assigning state with primitives:

	obj.count = $state(0);

svelte compiles into something like this:

	let count_1 = $.state(42);

	Object.defineProperties(obj, {
		count: {
			get() {
				return $.get(count_1);
			},
			set(v) {
				$.set(count_1, $.proxy(v, null, count_1));
			},
			enumerable: true
		}
	});

and for assigning state with objects:

	obj.counter = $state({count: 42})

svelte compiles into something like this:

	let counter_1 = $.state($.proxy({ count: 42 }));

	Object.defineProperties(obj, {
		counter: {
			get() {
				return $.get(counter_1);
			},
			set(v) {
				$.set(testCount, $.proxy(v, null, counter_1));
			},
			enumerable: true
		}
	});



And, for a state object defining custom getters and setters for a property doesn't work.

Please can you create a separate issue for that, seems like a bug.

👍 done! #14320

@trueadm
Copy link
Contributor

trueadm commented Nov 16, 2024

Just for clarity, what I was aiming for is for svelte to perform a code transformation when assigning state directly.

That's far too magical and simply assumes too much. If obj already has a setter for obj.count then that should be invoked rather than defining a new property. Not to mention if it was something like this obj[i] = $state(…) if obj is an array then you're going to cause all sorts of problems with length etc.

Oh and Object.defineProperty is absolutely terrible for performance, so it's best to avoid in hot paths.

@jdgamble555
Copy link
Author

@dummdidumm @trueadm - On the original issue, is it possible to get the return value to work?

Thanks!

J

@Leonidaz
Copy link

Leonidaz commented Dec 8, 2024

Oh and Object.defineProperty is absolutely terrible for performance, so it's best to avoid in hot paths.

FWIW, Object.defineProperty seems to be outperforming setting properties directly on a Proxy. The fastest way is to set a direct property on an non-proxied object.

https://www.measurethat.net/Benchmarks/Show/32930/5/objectdefineproperty-vs-objectassign-vs-assign-to-proxy

The Object.defineProperties(plural), on the other hand, yeah that one is slow

@trueadm
Copy link
Contributor

trueadm commented Dec 8, 2024

Direct object property is almost double the test for me

@Leonidaz
Copy link

Leonidaz commented Dec 8, 2024

https://www.measurethat.net/Benchmarks/Show/32930/5/objectdefineproperty-vs-objectassign-vs-assign-to-proxy

weird, on a Proxy?

This is what I'm getting on Chrome Version 131.0.6778.109 (Official Build) (x86_64)

image
      Model Name: MacBook Pro
      Model Identifier: MacBookPro16,1
      System Version: macOS 15.1.1
      Processor Name: 8-Core Intel Core i9
      Processor Speed: 2.4 GHz
      Number of Processors: 1
      Total Number of Cores: 8
      L2 Cache (per Core): 256 KB
      L3 Cache: 16 MB

Latest Safari seems to be faster but the results are pretty much the same:

image

Direct property on a POJO, just below these two, is definitely the fastest.

@trueadm
Copy link
Contributor

trueadm commented Dec 8, 2024

It appears you've not setup the Proxy properly in your benchmark, you're only handling set, but not defineProperty?

@Leonidaz
Copy link

Leonidaz commented Dec 8, 2024

It appears you've not setup the Proxy properly in your benchmark, you're only handling set, but not defineProperty?

I was just testing pass throughs without traps. But yeah, if we add a trap for defineProperty that will slow it down vs direct prop assignment on proxy that doesn't go through this trap. I'm not sure though why add a trap for defineProperty? Unless it's in dev for validation.

with defineProperty, get, set traps:
https://www.measurethat.net/Benchmarks/Show/32930/6/objectdefineproperty-vs-objectassign-vs-assign-to-proxy

@trueadm
Copy link
Contributor

trueadm commented Dec 8, 2024

It appears you've not setup the Proxy properly in your benchmark, you're only handling set, but not defineProperty?

I was just testing pass throughs without traps. But yeah, if we add a trap for defineProperty that will slow it down vs direct prop assignment on proxy that doesn't go through this trap. I'm not sure though why add a trap for defineProperty? Unless it's in dev for validation.

with defineProperty, get, set traps: https://www.measurethat.net/Benchmarks/Show/32930/6/objectdefineproperty-vs-objectassign-vs-assign-to-proxy

Our internal proxy implementation has it, as we don't allow modifications to the underlying target object.

@jdgamble555
Copy link
Author

@trueadm - Any note on my original post?

Could we still permit return $state() to work?

Thanks!

J

@Ocean-OS Ocean-OS linked a pull request Apr 6, 2025 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants