Svelte5: reactive Set constructor not working correctly about svelte HOT 14 CLOSED

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.

from svelte.

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)

from svelte.

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

from svelte.

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)

from svelte.

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.

from svelte.

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.

from svelte.

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

from svelte.

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

from svelte.

@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.

from svelte.

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.

from svelte.

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.

from svelte.

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.

from svelte.

See #11287 for a potential solution

from svelte.

This works now

from svelte.