Closed FoHoOV closed 3 months ago
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 :)
EDIT: apparently the console
api is not standardized and there's no assurance that changing toString
will customize how it prints an object.
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)
This is caused by #11200. The following are broken, I believe: forEach
, isDisjointFrom
, isSubsetOf
, isSupersetOf
, difference
, intersection
, symmetricDifference
, and union
.
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)
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.
The fix is to revert #11200 or implement all the methods that are currently implemented by calling
Set
methods applied to theReactiveSet
. Since that change removessuper
calls, the parentSet
doesn't have any data, and those methods are being called on an emptySet
(orMap
).Or, don't extend
Set
and haveReactiveSet
have aSet
rather than be aSet
. This will break the next time a method is added toSet
(orMap
orDate
orURL
orURLSearchParams
) 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.
It's not only more work, it's not forward-compatible.
@harrisi what about wrapping the native set
, url
and etc in a proxy instead?
@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.
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.
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.
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 aReactiveSet
without extendingSet
, so it won't be aSet
, but look like it. It would still work in many situations, exceptinstanceof 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:
ReactiveSet
inherits from Set
it will be forward-compatible by newest JS standards but might break what svelte users assume it doesReactiveSet
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.
See https://github.com/sveltejs/svelte/pull/11287 for a potential solution
This works now
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
System Info
Severity
blocking an upgrade