fix: use `state` instead of `source` in reactive classes #16239

paoloricciuti · 2025-06-25T16:25:12Z

So the problem with using source is that:

an $effect/$derived that reads a source created in itself will also depend on it...since normally state created in a reaction doesn't depend on itself this is counter intuitive
if you try to write to a source within the same derived it will throw an error

This is a bit problematic in some cases because we create the sources lazily...in the case of Map for example we don't create a source unless you get or has or set that first. However this means that if you create a new Map in a derived ask if it has something, then you try to write to it in the same derived the newly created source throws the error.

For set it was as simple as changing source to state...however with get and has it's a bit different: the effect needs to have a dependency on the source so state is out of question but then you don't want writes to it to throw. For that my solution was to add the source to the reaction_values after the get (only if the sources was newly created). For #read_all it was a bit more complex and i had to create a WeakSet for the newly created signals (not sure about memory/performance impact of this tho).

I also changed the source's to state's in Tweened and Spring since that also could've been problematic in cases like this

<script>
	import { Spring } from "svelte/motion";
	const map = $derived.by(() => {
			const spring = new Spring(0);
			spring.set(100);
			return spring;
	});
</script>

{map.current}

source is also used inside createSubscriber and I think it could have a problem there too but i need to check to be sure.

It's also used inside await, each and other blocks that "create" a value but i think for that it's probably fine.

As I've said in the issue I'm not satisfied with this solution since it feels a bit hack-ish to add the dependency and still allow the user to write to it.

I also need to add a bunch of tests for this but wanted to give the code to @gyzerok

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

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
Prefix your PR title with feat:, fix:, chore:, or docs:.
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.
If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

Run the tests with pnpm test and lint the project with pnpm lint

changeset-bot · 2025-06-25T16:25:19Z

⚠️ No Changeset found

Latest commit: e3072c9

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

svelte-docs-bot · 2025-06-25T16:25:41Z

Preview: https://svelte-dev-git-preview-svelte-16239-svelte.vercel.app/

github-actions · 2025-06-25T16:27:06Z

Playground

pnpm add https://pkg.pr.new/svelte@16239

paoloricciuti · 2025-06-25T17:13:20Z

New idea I'll try to implement: store the active reaction in the constructor of the Map/Set and use a state only if it's === to the current active reaction

gyzerok

Great work!

gyzerok · 2025-06-26T04:08:53Z

packages/svelte/src/reactivity/map.js

@@ -56,13 +56,17 @@ export class SvelteMap extends Map {
 	#sources = new Map();
 	#version = state(0);
 	#size = state(0);
+	/**@type {Reaction | null} */
+	#initial_reaction = null;


Wondering if there are cases when map still lives, while the reaction could have been GCed and this would prevent it?

Yeah I was wondering about that too...technically it shouldn't be the case because either there's no active reaction (it's outside of a derived/effect) or it's inside it but this means that when the derived/effect is no longer used the function should be GCd and everything inside it too...but I need to do a better check

Maybe this can become initial_reaction = WeakSet(active_reaction) and we can check with this.initial_reaction.has(active_reaction)?

Just for the sake of history, I've also discovered there is a WeakRef struct that could be alternative here. In case of any future issues might be a possible alternative implementation

gyzerok · 2025-06-26T04:15:48Z

Also tried installing this version locally, enabling runes mode forcefully and clicking through the interface. Looks like not hitting unsafe_state_mutation anywhere anymore.

…asses

Rich-Harris · 2025-07-07T18:26:01Z

packages/svelte/src/reactivity/map.js

+			teardown(() => {
+				this.#initial_reaction = null;
+			});


presumably if there's a situation that would create a memory leak with this (which I guess means smuggling a reference to the map to something outside the current reaction?), teardown only mitigates it, since the reaction in question might never be torn down, if it's an unowned derived.

do we therefore need a way to run code as soon as the current reaction has finished updating? #16316

The alternative is to use a weak ref to reference the reaction. Perf impact should be close to zero since it only applies once for new sources

We could also use WeakRef, we were discussing with Simon this morning that this seems the perfect use case for it

I've pushed a commit to use WeakRef since it's way easier than handling #16316 for this edge case and this is kinda the perfect use case for it. We can always revert if we don't feel like going for it.

Not sure I love the WeakRef solution tbh — it increases overall memory usage (since every map/set now has to have a WeakRef instance) and slows down every #source call, since WeakRef is known to impact performance

We tested with @dummdidumm today and it was at least 90% of normal access...also as @dummdidumm said this is really only accessed when we create the source for the first time.

If you think merging #16316 it's better we can also do that (or just ignore the edge case which btw should also be present in proxy)

second that, WeakRef only has theoretical perf implications here. It only applies when you create a source anew, which isn't that often compared to the reads in the vast majority of cases. I suspect that creating teardown or having an array like in #16316 (I think it's overkill, would just use the teardown) is the same memory overhead.

We can also determine this "not worth it" and live the potential memory leak. It can only leak if you create the map inside effect context A and then pass it outside to another context, and context A is destroyed but not the context it's passed to. Pretty sure our proxy has the same leak because we never clean up the original context either in case it's gone, and I haven't heard complaints.

I don't think the memory consumption is comparable at all — the array (and the callbacks it contains) are short-lived, while the WeakRefs live as long as their owners do.

But yeah I would just live with the edge case honestly. As with proxies it's only 'leaky' if the map/set is itself leaked, and you really have to work at it.

Would disagree with "keeping" the leak. I'd say that in this specific case it's not intuitive at all for the developer that by doing code like the following they introduce a memory leak:

$effect(() => { foo.bar = new SvelteMap() })

As someone who is developing apps with long-lived browser tabs I'd say that leaks are the most difficult things out there to find and fix. Generally in a lot of cases it's clear from the code and knowledge about how GC works to understand that hey, we are keeping a reference. But in this specific case the code looks like it shouldn't leak, so it's what is called "hard to reason about". It is actively messing with my expectations about it.

Yeah, this code might be weird, and I can't come up with a reason to do something like that, but it doesn't mean some developer won't find a need for it.

Consider it to be the same as previous values in teardown. It's not about if it's right or wrong, it's about the expectations and not making it confusing on the DX side of things

fix: use state instead of source in reactive classes

de8053e

fix: use active_reaction as indication to use source or state

2e69ed4

paoloricciuti marked this pull request as ready for review June 25, 2025 21:19

gyzerok reviewed Jun 26, 2025

View reviewed changes

fix: cleanup #initial_reaction on teardown to free memory

fb627c0

gyzerok mentioned this pull request Jun 28, 2025

(0 , reaction.fn) is not a function in update_reaction #16191

Closed

Merge remote-tracking branch 'origin/main' into source-to-state-in-cl…

2ea891e

…asses

Ocean-OS approved these changes Jul 7, 2025

View reviewed changes

paoloricciuti and others added 2 commits July 7, 2025 11:09

fix: use #source in set too

5670a21

unused

f261686

Rich-Harris reviewed Jul 7, 2025

View reviewed changes

chore: use WeakRef

e3072c9

Uh oh!

fix: use state instead of source in reactive classes #16239

Are you sure you want to change the base?

fix: use state instead of source in reactive classes #16239

Conversation

paoloricciuti commented Jun 25, 2025

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

Tests and linting

Uh oh!

changeset-bot bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

svelte-docs-bot bot commented Jun 25, 2025

Uh oh!

github-actions bot commented Jun 25, 2025

Uh oh!

paoloricciuti commented Jun 25, 2025

Uh oh!

gyzerok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyzerok commented Jun 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummdidumm Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyzerok Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyzerok Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fix: use `state` instead of `source` in reactive classes #16239

fix: use `state` instead of `source` in reactive classes #16239

changeset-bot bot commented Jun 25, 2025 •

edited

Loading

dummdidumm Jul 7, 2025 •

edited

Loading

gyzerok Jul 8, 2025 •

edited

Loading

gyzerok Jul 8, 2025 •

edited

Loading