fix: ensure signal graph is consistent before triggering $inspect signals

[trueadm]

Fixes #13146.

Sync effects are not possible with Svelte 5's push-pull system because they can make can break the reactive graph – which is also why we don't permit writes within a derived too. The same problem is occurring here, as inspect effects are run sync – they are actually happening as part of an existing derived – which means they're a bit like writes in a derived and can cause tearing to the reactive signal graph. To avoid this we can call flushSync just before invoking these effects, as that should ensure the graph is made consistent again.

Also another issue I found was that we don't "reset" the inspect_effects Set when we enter a derived – which we should as a derived can create it's own local state that should have no bearing on the parent inspect effect.

@paoloricciuti If you could help me create a minimal test case for this, that would be great.

[changeset-bot]

🦋 Changeset detected

Latest commit: ae52e43

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

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

[paoloricciuti]

@paoloricciuti If you could help me create a minimal test case for this, that would be great.

I need to run now but i'll try this evening if i can

[paoloricciuti]

Uh trying to write a test i actually realised that my assumption was probably wrong (i mean maybe not totally but the actual problem is probably a combination)...

<script>

	class Val{
		val = $state();

		constructor(val){
			this.val = val;
		}
	}

	class Double{
		count = $state(0);
		der = $derived(new Val(this.count));

		constructor(count){
			this.count = count;
		}
	}
	
	let doubles = $state([]);

	const doubles_val = $derived(doubles.map(double => double.der));

	$inspect(doubles_val);
</script>

<button onclick={()=>{
	doubles.push(new Double(2));
}}>
</button>

REPL

Here WE ARE WRITING IN A DERIVED! It's subtle but by creating a new Val() inside derived we are assigning to this.val inside the constructor (which is state)

[trueadm]

Uh trying to write a test i actually realised that my assumption was probably wrong (i mean maybe not totally but the actual problem is probably a combination)...

<script>

	class Val{
		val = $state();

		constructor(val){
			this.val = val;
		}
	}

	class Double{
		count = $state(0);
		der = $derived(new Val(this.count));

		constructor(count){
			this.count = count;
		}
	}
	
	let doubles = $state([]);

	const doubles_val = $derived(doubles.map(double => double.der));

	$inspect(doubles_val);
</script>

<button onclick={()=>{
	doubles.push(new Double(2));
}}>
</button>

REPL

Here WE ARE WRITING IN A DERIVED! It's subtle but by creating a new Val() inside derived we are assigning to this.val inside the constructor (which is state)

You can write to state that is created locally in the derived. If you couldn't you'd get given an error.

[paoloricciuti]

You can write to state that is created locally in the derived. If you couldn't you'd get given an error.

However if you write that in a micro task the error from inspect it's not there anymore

[paoloricciuti]

<script>
	class Val{
		val = $state();
		constructor(val){
			this.val = val;
		}
	}
	
	let doubles = $state([]);

	const doubles_val = $derived(doubles.map(num => new Val(num)));

	$inspect(doubles_val);
</script>

<button onclick={()=>{
	doubles.push(2);
}}>
</button>

This also causes the error

[trueadm]

@paoloricciuti Are you running these against this PR, as none of them error for me?

[paoloricciuti]

@paoloricciuti Are you running these against this PR, as none of them error for me?

Oh no in your PR is fine, sorry maybe I wasn't clear enough...I was trying to write a minimal test that was failing on main and passing here

[ottomated]

Is there a good reason for $inspect to be ran sync? It makes more sense for its behavior to match $effect to prevent confusion in my opinion

[trueadm]

Is there a good reason for $inspect to be ran sync? It makes more sense for its behavior to match $effect to prevent confusion in my opinion

It can take a sub-rune $inspect(x).with which means you can add a console.trace or even a debugger and catch the mutation inline. So it needs to be sync. https://svelte-5-preview.vercel.app/docs/runes#$inspect

[ShaitanLyss]

Is latest deployment how you access this pull request? If so, the original repl still doesn't work when $inspect runes are present.

[trueadm]

Is latest deployment how you access this pull request? If so, the original repl still doesn't work when $inspect runes are present.

Since been fixed.

[dummdidumm]

In another PR we fixed a custom elements bug which was caused by calling flushSync inside the wrapper, which meant things were a bit out of order. Could something like this happen here, too, because flushSync is called at an unfortunate time?

[trueadm]

@dummdidumm Do you remember more about that PR? Are you suggesting that we could invoke the same bug because we call flushSync?

[dummdidumm]

I'm talking about #12787 and the referenced issue. There the problem was flushSync called at the wrong times. I'm wondering whether we potentially have the same problem here.

[trueadm]

@dummdidumm I revised the logic so that we only flush if there's no current effect – that should alleviate the issue you mention.