-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat: generic utility for making any class reactive #11504
base: main
Are you sure you want to change the base?
Conversation
|
This is still in draft mode because in this code: <script>
import {Set} from "svelte/reactivity";
const data = $state([1,2,3]);
const reactiveSet = new Set(data);
$effect(() => {
console.log("has 1", reactiveSet.has(1));
});
$effect(() => {
console.log("has 2", reactiveSet.has(2));
});
$effect(() => {
console.log("has 3", reactiveSet.has(3));
});
$effect(() => {
console.log("has 5", reactiveSet.has(5));
});
</script>
<button onclick={()=>{
reactiveSet.delete(2);
}}>
delete 2
</button>
<button onclick={()=>{
reactiveSet.add(2);
}}>
add 2
</button>
<button onclick={()=>{
reactiveSet.clear();
}}>
clear
</button>
when we delete <script>
import {Set} from "svelte/reactivity";
const data = $state([1,2]);
const reactiveSet = new Set(data);
$effect(() => {
console.log("has 1", reactiveSet.has(1));
});
$effect(() => {
console.log("has 2", reactiveSet.has(2));
});
</script>
<button onclick={()=>{
reactiveSet.delete(2);
}}>
delete 2
</button> after clicking on "delete 2" only |
As you saw, the general problem with this approach is that the object isn't fine-grained. You want $effect(() => {
console.log(reactiveMap.get(42));
}); run only when a value with the key 42 is added or set but not any other. There are also some debates about whether the stored item should be reactified. |
I feel like this is quite nice generic solution for making arbitrary things reactive easily (e.g. objects from 3rd party libraries that you have no control over). However as @7nik pointed out, this isn't fine-grained. So, I think that basic things like Set/Map should be implemented manually, to make them as fine-grained as possible. |
@7nik @Azarattum, I completely agree with you. I've marked this as a draft while I try to comeup with a solution (hopefully) that addresses this issue. However, as noted in issue #11515, the current implementation still faces a similar challenge from a different angle. For example, if a <script>
import {Set} from "svelte/reactivity";
const data = [1];
const reactiveSet = new Set(data);
$effect(() => {
console.log("has 1", reactiveSet.has(1));
});
$effect(() => {
console.log("has 2", reactiveSet.has(2));
console.log("has 3", reactiveSet.has(3));
// and so one for another 100000 elements that dont exist in the set and might never do!
});
</script> basically each |
Well, to react to a 1000 items changing finely, we would need a 1000 signals. I think in this case this is what the user has requested explicitly, so it feels like an expected behavior. I think having a lot of signals that trigger less is better than a few that trigger often. This is what the fine-grainness is all about. Though I think that it would still be more common to have less signals than items in a set. So, lazy signal initialization shouldn't be an issue. |
I thought about storing in Map super items of shape Regarding reactivity for non-existing keys, just creating and storing signals for them can cause a memory leak when somebody uses huge objects as keys or adds and removes thousands of unique keys. Temporary returning derived (kind of |
@7nik wouldn't this be a good candidate for a WeakMap for storing the signals, to not hold strongly to the user's potentially huge Map key or Set value? |
fixed some issues thanks to the comments above, also fixes #11515 |
31800ee
to
1457398
Compare
I came up with better idea to make it more accurate. Because currently it makes signals aware that a change "is going to happen", the correct implementation would be "a change has happened". |
I've not had time to look into this deeply yet, will get around to it this week! Also, the lint CI workflow is failing :) |
Reallyyyy looking forward for your feedback >_< |
…anged within a single changeset or not
…ifying read methods
…a new instance on each change
1a7213c
to
d3fd750
Compare
@trueadm Yes from memory perspective this implementation could actually retain more memory as it goes on. But maybe doing these solutions can help?
If I actually opt into using a reactive set/map/url/date/..., I would expect it to be reactive just like everything else in svelte (this package is comming from svelte as well). So I think not being reactive when keys are objects, loosing reactivity when we clear a set/map or other things mentioned in #11727 might not be an expected behavior. The challenge is we really need to know which read function is called with what paremeter(s) (or getters with no parameters), so it could be reactive based on exactly that (or somebody else affecting it). But we might be able to group some read-methods to use the same set of signals. For instance we can group has/get into one set of signals. What I mean is do you think its worth trying to optimize this or the memory cost just doesn't worth it? My highlights on this:
|
It’s more important to be efficient with memory than have greater fine grained reactivity. We can’t create signals only in reactive contexts either, we used to do that for objects and arrays but it was causing bugs so we stopped doing it. The truth is using a dynamic wrapper is always going to be more expensive than doing it statically on the prototype as the JIT can optimize it better. Performance of runtime is even more critical than the theoretical benefits of fine grain performance - the best pattern is actually somewhere in the middle. That’s why we share a single effect for as much the template as possible rather than have many small fine grain ones. |
@trueadm Thanks for the review!! My whole mindset was around making stuff as fine-grained as possible and not worrying about memory at that moment then try to make it performant in later step xD I was thinking in THE OPPOSITE direction🫡dayumn
|
@FoHoOV Signals aren't free at the end of the day, we have to do a lot of bookkeeping around them to manage their relationships. If we're doing this for things that might leak, then it's surely not worth it. Thank you for digging into this, you've been awesome :) We still need to fix Date though right? How do you feel about tackling that without the usage of a wrapper? |
@TrueDM Thanks! Sure, if by the time I start nobody has done it yet, I'll try to do it for the url, date and searchparams. |
Does Date need to be fixed? I see Date as a wrapped timestamp with a bunch of methods to format/modify it. So, it doesn't seem efficient to create signals (even if on-demand only) for all read methods. And it is easy to stop over-reactivity with $derived on userland. |
This is a completely new take on the reactivity package. Right now, if you want to make a class reactive, you have to create a subclass and manually set up signals for each property. For classes like
Set
orMap
, you even have to duplicate the data in an innerSet
orMap
, which can lead to bugs and bad memory management. This PR is inspired by #11287 and the current implementation.Goal
Let’s break down how it works. Every class has properties we read from and properties we write to, similar to the current implementation.
Read Properties:
set.size
,map.keys()
, andset.values()
don’t take any parameters and should be reactive based on actual changes.set.get(x)
,map.get(x)
, orsomething.someMethod(x, y)
depend on one or more parameters and should be reactive based on those parameters and certain conditions.url.origin
orurl.hash
don’t take any parameters but are reactive based on certain conditions rather than actual changes. For example, settingurl.hash
doesn’t changeurl.origin
, and vice versa.Write Properties:
set.clear()
don’t take any parameters and could cause reactivity based on some conditions or just cause reactivity with each call.set.add(x)
,map.set(x, y)
, orurl.href = x
take a parameter and should cause reactivity based on that parameter.Each write property could cause reactivity. Based on this, we have two types of reactivity inside classes:
With these scenarios, we can create a utility that covers everything mentioned. Here’s a code sample to explain what it does:
This has three main parts:
notify_read_methods
.write_properties
is invoked.The magic happens in the interceptors. They give us the current value (before the change happens), the parameters that the write property is invoked with (if any), and a method called
notify_read_methods
that can be invoked for any property listed inread_properties
.Breaking Down the
clear
Interceptor:clear
method, we don't want to cause reactivity if the set is already empty, so we return early.has
read methods that are already registered. For instance, if we never calledset.has(1000)
, why bother?size
. As mentioned earlier, anything not listed inread_properties
will be notified automatically based on any change. If the interceptor returns false, we won't increment an internal signal calledversion
(just like the current implementation). But if it returns true or we callnotify_read_methods
, it tells the utility to increment the version signal so other properties not listed inread_properties
can get notified.Breaking Down the
add
Interceptor:x
), return early.has
functions called withx
. This will also increment the internalversion
signal, which will notifysize
,values
, etc.notify_read_methods
and just returnedtrue
, that would also increment theversion
signal.With this setup, it's easy to be as fine-grained as possible without data duplication and make any class reactive in a generic way without worrying about data management. All you care about is who notifies who and what should be reactive.
Relates to #11200, #11385, #11233, #11235 and #11287.
Fixes #11727, the log problem part of (#11233) and #11680
Svelte 5 rewrite
Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint