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

Svelte5: reactive Set constructor not working correctly #11222

Open
FoHoOV opened this issue Apr 18, 2024 · 13 comments · May be fixed by #11287
Open

Svelte5: reactive Set constructor not working correctly #11222

FoHoOV opened this issue Apr 18, 2024 · 13 comments · May be fixed by #11287
Milestone

Comments

@FoHoOV
Copy link

FoHoOV commented Apr 18, 2024

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

paoloricciuti commented Apr 18, 2024

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

FoHoOV commented Apr 18, 2024

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

harrisi commented Apr 18, 2024

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

@paoloricciuti
Copy link
Contributor

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

harrisi commented Apr 18, 2024

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

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

harrisi commented Apr 18, 2024

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

@FoHoOV
Copy link
Author

FoHoOV commented Apr 18, 2024

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

@paoloricciuti
Copy link
Contributor

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

FoHoOV commented Apr 18, 2024

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

harrisi commented Apr 18, 2024

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

FoHoOV commented Apr 19, 2024

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.

@Rich-Harris Rich-Harris added this to the 5.0 milestone Apr 21, 2024
@Azarattum Azarattum linked a pull request Apr 22, 2024 that will close this issue
5 tasks
@Azarattum
Copy link
Contributor

See #11287 for a potential solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants