feat: generic utility for making any class reactive

[FoHoOV]

This is a completly new take on the reactivity package. After issues like #11222 and solutions like #11200, #11385 and #11287 we still have data duplication and a complex solution (also some other issues which makes current implementation not fine grained like #11515). This implementation allows us to make any (hopefully) builtins reactive without any code repetition in more flexible and generic way. This we only care about notifying the read methods about changes and not the data management itself. This pr also uses the #11287 idea to only create signals when they are actually read, not on initialization which could be a performance buff - for instance in my tests with a map which has 1_000_000 elements, the page load speed increased by 1s.

if the idea is accpeted and actually good I'll refactore other reactivity package classes to use this utility instead.

using this utility on Set for instance resulted in:

export const ReactiveSet = make_reactive(Set, {
	write_properties: ['add', 'clear', 'delete'],
	read_properties: ['has'],
	interceptors: {
		add: (notify_read_methods, value, property, ...params) => {
			if (value.has(params[0])) {
				return false;
			}
			notify_read_methods(['has'], params[0]);
			return true;
		},
		clear: (notify_read_methods, value, property, ...params) => {
			if (value.size == 0) {
				return false;
			}
			notify_read_methods(['has'], NOTIFY_WITH_ALL_PARAMS);
			return true;
		},
		delete: (notify_read_methods, value, property, ...params) => {
			if (!value.has(params[0])) {
				return false;
			}
			notify_read_methods(['has'], params[0]);
			return true;
		}
	}
});

Svelte 5 rewrite

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• [x] Prefix your PR title with feat:, fix:, chore:, or docs:.
• [x] This message body should clearly illustrate what problems it solves.
• [x] Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• [x] Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: d3fd750491ae48a5f4fc3fde8467d702fed0e5ad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[FoHoOV]

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 2 from the set, all effects run where we call reactiveSet.has(SOMETHING). Keeping in mind that delete on a value that doesn't exist will not rerun the effects or deleting a value that exists twice doesn't rerun the effect for the second time. Following the current behaviour:

<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 reactiveSet.has(2) should get called (or things simillar to this case). Everything else works.

[7nik]

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. You can turn each reading method into a derived so that the effect won't rerun, but then, in a set/map with 10k items, each change will trigger 10k derives, which isn't performant. So, the most performant way is again to have a signal for each item, but you pay with memory for it.

There are also some debates about whether the stored item should be reactified.

[Azarattum]

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.

[FoHoOV]

@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 set contains a single item and we execute set.has(X) 1000 times within an effect, where X is not an element of the set, with each modification it results in at most 1000 has calls that haven't changed. Unless each of those have their own signals which results in 1000 signals that might be an unnecessary overhead. I'm honestly not sure how to resolve this effectively. Any ideas?

<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 read like method might create many unnecessary signals.

[Azarattum]

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.

[7nik]

I thought about storing in Map super items of shape { signal, value }, but this approach won't work for Set. So, I lean toward storing values in the super and signals in a private map. Plus, it allows not to create signals for keys that were never read.

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 $deirvied( (this.#version, super.has(key)) )) may cause bad performance when somebody watches for thousands of non-existing keys simultaneously (though how likely it is?). Another alternative is StartStopNotifier, which removes the signal when nobody listens to it anymore.

[Azarattum]

@7nik, https://github.com/sveltejs/svelte/pull/11287 does kind of implement StartStopNotifer pattern. Though the implementation itself still isn't very elegant...

[03juan]

So, I lean toward storing values in the super and signals in a private map. Plus, it allows not to create signals for keys that were never read.

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.

@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? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap#emulating_private_members

[FoHoOV]

fixed some issues thanks to the comments above, also fixes #11515

[FoHoOV]

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". Edit: Fixed now

[trueadm]

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 :)

[FoHoOV]

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 >_< also the linter issue is so weird, because that should actually work. used another way to fix it though

[FoHoOV]

@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

[FoHoOV]

I updated the PR description to include a detailed explanation of what this change does.

[Azarattum]

@FoHoOV, I really like your PR so far! I've taken a quick look into it. It seems that you've adopted the lazy signal initialization model from #11287, which is great for finer grained reactivity. However, you've faced the same challenge as my PR of cleaning up unused signals (this was my attempt to tackle it). As it seems, you haven't currently addressed this issue in your implementation. Therefore the following code would cause a memory leak:

const set = new ReactiveSet([1, 2, 3]);

const cleanup = effect_root(() => {
  render_effect(() => {
    set.has(42); // we have created a signal for `has 42` here
  });
});

cleanup(); // the effect is no longer
// at this point we have a memory leak, since nobody listens to `has 42`,
//   but #read_methods_signals still has it

This would be a common case since component unmounts cause effect cleanups. Especially considering that we create signals for values that could not be in a Map/Set, the memory leak could be pretty substantial.

[FoHoOV]

@Azarattum I'm very happy that you liked it, thanks ❤️!

const set = new ReactiveSet([1, 2, 3]);

const cleanup = effect_root(() => {
  render_effect(() => {
    set.has(42); // we have created a signal for `has 42` here
  });
});

cleanup(); // the effect is no longer ...

As far as I know there is no good/clean way to determine that. BUT I actually did test this case and I have good news! as soon as the reactive class goes out of scope and gc is called, everything in read_methods_signals will be cleaned up (if nobody else is holding a reference)!

The test

First we have to change the code a little bit to test if stuff is actually getting cleared or not. So lets do that.

1. in utils.js create a FinalizationRegistry

const registry = new FinalizationRegistry((heldValue) => {
	console.log(`garbage deleted! ${heldValue}`);
});

2. in utils.js change get_read_signals to this:

function get_read_signals(version_signal, read_methods_signals, property, options, ...params) {
	if (options.read_properties?.includes(property)) {
		(params.length == 0 ? [null] : params).forEach((param) => {
                       // ------------- THIS PART IS FOR THE TEST
			console.log('registered');
			registry.register(param, 'param cleaned up');
                        // -------------
			// read like methods that are reactive conditionally should create the signal (if not already created)
			// so they can be reactive when notified based on their param
			const sig = get_signal_for_function(read_methods_signals, property, param, true);
			get(sig);
		});
	} else if (!options.write_properties.includes(property)) {
		// other read like methods that are not reactive conditionally based their params and are just notified based on the version signal are here
		get(version_signal);
	}
}

Using a REPL similar to this on a build from this PR, you will see 1000 logs for garbage deleted when clicking on the toggle button so I think thats fine. Everything is bound to the lifecycle of set which is good enough I guess.

NOTE: for some reason FinalizationRegistry only works on firefox.

Just use a WeakMap

Since you might want to notify a method with all registered params I can't do that:

if (params.length == 1 && params[0] == NOTIFY_WITH_ALL_REGISTERED_PARAMS) {
	read_methods_signals.get(name)?.forEach((sig) => {
		increment_signal(initial_version_signal_v, version_signal, sig);
	});
} 

You could change the implementation a bit to use weakmap for objects and a map for primitives:

#read_methods_signals = {
	primitives: new Map(), // notify_with_all_registered_params could be here
	objects: new WeakMap(), // for objects we will not hold a strong reference to that object
};

but does it worth it?

Thoughts?

[Azarattum]

@FoHoOV, I don't think WeakMap can solve the issue here. Keys in Map are usually strings, so that wouldn't make much difference. The fact that signals are collected when the Map/Set is dropped is fairly obvious and definitely isn't the issue I've highlighted. The problem when we have is a global (or in parent component) reactive Map/Set and a bunch of children that constantly get created, listen to something in that Map/Set and unmounted. In this case it would cause the signals grow proportional to subscribers ever created opposed to active ones.

I think this can be addressed by adapting the core logic to support such behaviour. We already have access to current_effect from runtime. We just need an elegant way to apply a teardown function that wouldn't be overwritten just after the effect execution. I've done this with creating a teardown function from a branching effect, but @trueadm pointed out that this approach isn't very efficient. Maybe we could extend the effect interface to allow for some kind of teardown collection (e.g. a Set of functions) that could be populated during the effect execution and not just by its return value.

[FoHoOV]

@Azarattum, sorry for the misunderstanding. I think I got what you meant, I just wanted to emphasise that read_methods_signals will not prevent the garbage collector from ever collecting them.

We already have access to current_effect from runtime. We just need an elegant way to apply a teardown function that wouldn't be overwritten just after the effect execution.

As you mentioned, based on that and this comment, we can do the following:

1. Avoid creating signals if we are not inside an effect or a derived state. I'm not that familiar with Svelte's internals, so what's the correct approach for this?
2. Remove unused param-to-signal key-value pairs.

These are optimizations that can happen generally in the current implementation or follow-up PRs to this (if accepted). Thinking about it though, these optimizations can happen for every class in a single place with this PRs implementation (the get_read_signals function).

[trueadm]

So I dug through this and also played around with it locally. I don't think it's the right direction for what we want to do here. I put up https://github.com/sveltejs/svelte/pull/11827 as a pointer towards how we can deal with some of the issues you try to tackle. Here's my feedback:

• Signals are created far too frequently for any key in a Map or value in a Set. Really they should just be primitives only. You can quite easily retain far too many values with this approach and I was able to see the memory heap grow as I added objects and delete them from my Maps and Sets.
• The overhead of a utility to generate the reactivity for the Map, Date and Set really kills performance for me. I'm seeing a lot of overhead doing basic operations on them compared to the current version on main. I also saw memory consumption was almost double for an almost empty Map.

I ultimately don't think a general purpose utility is the right thing for core classes that need the best in class performance and memory. Thanks for looking into this and digging into some of the complexities. I think your utility function could be really handy as a third-party resource and also for others to learn from :)

I do wonder if you'd like to revisit fixing the URL reactivity class though? I don't think a generic utility is the right approach here, but maybe you have an alternative approach you might want to try out

[FoHoOV]

@trueadm Yes from memory perspective this implementation could actually retain more memory as it goes on. But maybe doing these solutions can help?

... based on that and https://github.com/sveltejs/svelte/pull/11287#issuecomment-2103807182, we can do the following:

1. Avoid creating signals if we are not inside an effect or a derived state. I'm not that familiar with Svelte's internals, so what's the correct approach for this?
2. Remove unused param-to-signal key-value pairs.

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:

1. being able to create actually fine-grained reactive classes from existing classes (without worring about data-management)
2. being able to fix/optimize stuff in a signle place (utils.js)
3. easy to use API which makes it easier to make other classes reactive in future

[trueadm]

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.

[FoHoOV]

@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

• Many thanks for giving me your time >_<

[trueadm]

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

[FoHoOV]

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

[7nik]

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.

[trueadm]

Closing as stale.