Svelte5: reactive Set constructor not working correctly

[FoHoOV]

Describe the bug

This reproduction sums it up basically. Calling the Set constructor from an array doesn't work. If it helps, it does work with production builds.

Reproduction

1- goto this REPL. 2- see the logs

Logs

"init"
"dataAsNativeSetFromState"
Set(4) { "1" ,"2" ,"3" ,"4" }
"init"
"dataAsNativeSetFromRaw"
Set(4) { "1" ,"2" ,"3" ,"4" }
"init"
"dataAsReactiveSetFromState"
Set(0) { }
"init"
"dataAsReactiveSetFromRaw"
Set(0) { }

System Info

svelte REPL on "Svelte v5.0.0-next.107"

Severity

blocking an upgrade

[paoloricciuti]

This is just how things are logged. it will probably just need to upgrade the toString method too.

However since this is blocking an upgrade for you go ahead and use it because the values are there :)

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE5WSTY-CMBCG_0ozesCESPbj5KpZL3vcAxythwbGtQm0TTtCDOG_bwoaV4Vl99B0mHnneduhNexljg4W2xqUKBAWsDEGQqCT8R-uxJwQQnD6aFOfWbrUSkNrrjjJwmhLrE6QGra3umD83BFZFCnJUtKJwxsnr_Yr1coRywQJtmJbDk8cQsbhudteuu2Vw-7tVr1xCQlCtmJT54PAJ2cPok9BssQE6cPq4tKhsGKVVJmu5glS8AM3AohF1d_-2Bd31-2zHvW867263tgpTlOpnMGUAg5D1_UDHKrND8IF7chn3SF-58WiGqDFohpn9U3kyuur_pt5c8LH2h1vGV1frqonTkljkFiuvwKHNGu8Yz1BkR6YQ2LCsVLkbZZTfQnryAvacHmwLFpzVUdnVNOC3y2qDG3LHfoT3mxMGItqQNY3u79JWyaEUOhM7iVmsCB7xGbXfAMec29nBwQAAA==

EDIT: apparently the console api is not standardized and there's no assurance that changing toString will customize how it prints an object.

[FoHoOV]

However since this is blocking an upgrade for you go ahead and use it because the values are there :) ...

see this repl as well.

As the docs mention, they should behave exactly the same, except this one is reactive. So I still beleive this is not intented.

<script>
	import {Set} from "svelte/reactivity";	
	
	const data = ["1", "2", "3", "4"];
	
	const nativeSet = new window.Set(data);
	const reactiveSet = new Set(data);

	// 4 elements get logged cause 4 elements in here
	nativeSet.forEach((a)=>console.log("n",a));

	// nothing gets logged cause nothing in here
	reactiveSet.forEach((a)=>console.log("r", a)); // expected to log "r 1 ... 4"

	// or if the console.log is not correct as @paoloricciuti says we can test it
	reactiveSet.forEach((a)=>console.log("something")); // expected to log "something" 4 times
</script>

I've commented what I expect from the console logs, don't you agree they are logical? (this the code from the REPL I've linked)

[harrisi]

This is caused by #11200. The following are broken, I believe: forEach, isDisjointFrom, isSubsetOf, isSupersetOf, difference, intersection, symmetricDifference, and union.

[paoloricciuti]

However since this is blocking an upgrade for you go ahead and use it because the values are there :) ...

see this repl as well.

As the docs mention, they should behave exactly the same, except this one is reactive. So I still beleive this is not intented.

<script>
	import {Set} from "svelte/reactivity";	
	
	const data = ["1", "2", "3", "4"];
	
	const nativeSet = new window.Set(data);
	const reactiveSet = new Set(data);

	// 4 elements get logged cause 4 elements in here
	nativeSet.forEach((a)=>console.log("n",a));

	// nothing gets logged cause nothing in here
	reactiveSet.forEach((a)=>console.log("r", a)); // expected to log "r 1 ... 4"

	// or if the console.log is not correct as @paoloricciuti says we can test it
	reactiveSet.forEach((a)=>console.log("something")); // expected to log "something" 4 times
</script>

I've commented what I expect from the console logs, don't you agree they are logical? (this the code from the REPL I've linked)

As @harrisi was saying this is a different problem from what you showcased in your first repl. I'll see if I can find a fix tomorrow (I don't know if I'll have time tho so if someone else wants to work on this go ahead)

[harrisi]

The fix is to revert #11200 or implement all the methods that are currently implemented by calling Set methods applied to the ReactiveSet. Since that change removes super calls, the parent Set doesn't have any data, and those methods are being called on an empty Set (or Map).

Or, don't extend Set and have ReactiveSet have a Set rather than be a Set. This will break the next time a method is added to Set (or Map or Date or URL or URLSearchParams) as well.

[paoloricciuti]

The fix is to revert #11200 or implement all the methods that are currently implemented by calling Set methods applied to the ReactiveSet. Since that change removes super calls, the parent Set doesn't have any data, and those methods are being called on an empty Set (or Map).

Or, don't extend Set and have ReactiveSet have a Set rather than be a Set. This will break the next time a method is added to Set (or Map or Date or URL or URLSearchParams) as well.

Yeah i also commented on the PR. Personally i would revert #11200 ...you could replicate those methods but then there's more work to maintain those classes for a small memory gain.

[harrisi]

It's not only more work, it's not forward-compatible.

[FoHoOV]

@harrisi what about wrapping the native set, url and etc in a proxy instead?

[paoloricciuti]

@harrisi what about wrapping the native set, url and etc in a proxy instead?

You would still need to call those methods on an actual set instance or JS will throw.

[FoHoOV]

and also, should it be forward compatible? What if standard adds a new method that mutates the original set in someway, it shouldn't be available in ReactiveSet right away, it might require some modifications.

[harrisi]

and also, should it be forward compatible? What if standard adds a new method that mutates the original set in someway, it shouldn't be available in ReactiveSet right away, it might require some modifications.

The current implementation would still let you call whatever new Set methods may be added but wouldn't do anything, or do something weird. One alternative would be to define a ReactiveSet without extending Set, so it won't be a Set, but look like it. It would still work in many situations, except instanceof Set would return false, and types get all weird.

This discussion is pretty spread out across different places, and really isn't specific to this issue, though.

[FoHoOV]

and also, should it be forward compatible? What if standard adds a new method that mutates the original set in someway, it shouldn't be available in ReactiveSet right away, it might require some modifications.

The current implementation would still let you call whatever new Set methods may be added but wouldn't do anything, or do something weird. One alternative would be to define a ReactiveSet without extending Set, so it won't be a Set, but look like it. It would still work in many situations, except instanceof Set would return false, and types get all weird.

This discussion is pretty spread out across different places, and really isn't specific to this issue, though.

But what if the "new methods" added to Set requires overriding the method to make it reactive? for instance lets say the add methods will be introduced tomorrow. You will use it assuming it will be reactive because it's comming from "svelte/reactivity" and it is a reactive Set. Current implementation allows you to do so but might introduce bugs and unexpected behaviour. My point is:

1. if ReactiveSet inherits from Set it will be forward-compatible by newest JS standards but might break what svelte users assume it does
2. if ReactiveSet doesn't inherit from Set but holds an internal Set instead it might not be up to date with what js standard does, but will be compatible with sveltes point of view.

I don't know which one is better.

[Azarattum]

See https://github.com/sveltejs/svelte/pull/11287 for a potential solution

[dummdidumm]

This works now