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 >_< |
…ort stuff from the base proxied class
bdc860f
to
705e9d5
Compare
@trueadm today I actually had time and changed other reactivity package classes to use the this utility as well. marking this as draft again till I complete those |
d58d6ce
to
aaf329a
Compare
…anged within a single changeset or not
I updated the PR description to include a detailed explanation of what this change does. |
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