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

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

That will not introduce a leak...to introduce a leak you would do some weird as Simon pointed out

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

Hm, not sure what I am missing here. Why it won't introduce a leak? As long as I hold to the map somewhere I hold to the $effect scope, no?

Duh I misread your code...yeah that will "leak" although only until foo is not garbage collected.

Yeah, so my point is that if I write to foo.bar I do understand that the map is held until foo.bar is garbage collected, but I definitely don't expect $effect and everything related to it (dependencies, nodes and everything related to components) to leak as well.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

Tests and linting

Uh oh!

Uh oh!

⚠️ No Changeset found

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

Uh oh!

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

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