svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Prevent effect_update_depth_exceeded when using bind:value on a select with deriveds state in legacy components

Open raythurnvoid opened this issue 5 months ago • 6 comments

Fixes https://github.com/sveltejs/svelte/issues/13768

The bug

In legacy mode Svelte generates extra code for every
<select bind:value={…}>.
That helper reads the bound value and immediately calls invalidate_inner_signals, which writes back to the signals it just read so that indirect updates propagate.

When the bound value itself is produced by a reactive statement
($:) this write-back creates a tight loop:

<script>
	const default_details = { country: '' };

	$: data    = { locked: false, details: null };
	$: details = data.details ?? default_details;   // reactive

	// variant that fails the same way
	/* let details;
	   $:{ details = data.details ?? default_details } */
</script>

<select bind:value={details.country} disabled={data.locked}>
	<option value="1">1</option>
</select>
  1. Helper reads details.country
  2. invalidate_inner_signals does internal_set(data, data)
  3. $: re-runs, re-assigns details
  4. Helper fires again → infinite loop
    runtime throws effect_update_depth_exceeded.

What this PR does

Stops emitting the helper when it’s not needed.

In setup_select_synchronization we now check the bound identifier:

// skip helper if the variable is already managed by a `$:` block
if (bound.type === 'Identifier') {
	const binding = context.state.scope.get(bound.name);

	// 1. declared directly inside `$:`
	if (binding?.kind === 'legacy_reactive') return;

	// 2. declared elsewhere but *assigned* inside any `$:` block
	for (const [, rs] of context.state.analysis.reactive_statements) {
		if (rs.assignments.has(binding)) return;
	}
}

If either condition is true the synchronisation helper is omitted, breaking the cycle. For all other bindings (plain state, props, store-subs, etc.) the helper is still generated, so existing behaviour should be unchanged.

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.
  • [x] If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

raythurnvoid avatar Jun 15 '25 01:06 raythurnvoid

🦋 Changeset detected

Latest commit: 0ceac964255a9b471f108be9d54bbd9192e44e59

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

changeset-bot[bot] avatar Jun 15 '25 01:06 changeset-bot[bot]

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

svelte-docs-bot[bot] avatar Jun 15 '25 01:06 svelte-docs-bot[bot]

Playground

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

github-actions[bot] avatar Jun 15 '25 01:06 github-actions[bot]

I wonder why invalidate_inner_signals is placed in a separate effect instead of calling it inside the selector's setter, similarly to how it was done in Svelte 4:

	function select_change_handler() {
		details.country = select_value(this);
		($$invalidate(0, details), $$invalidate(1, data));
	}

7nik avatar Jun 15 '25 17:06 7nik

Mmh interesting, I can try to change my solution to do that and see if it works ^^.

So to mimic the svelte 4 behavior we should call it inside the mutate callback of the value binding right?

raythurnvoid avatar Jun 15 '25 17:06 raythurnvoid

setup_select_synchronization is such, basically, since the very beginning of Svelte 5 and I'm not sure why. But I'd explore moving the logic inside the setter in hope we can recreate the Svelte 4 behavior.

7nik avatar Jun 15 '25 18:06 7nik

Thank you! I think @7nik is right — the issue here is that we're using effects at all for this. I worry that tweaking the logic will fix the test case but still be buggy. Opened #16200 to start looking into this

Rich-Harris avatar Jun 18 '25 21:06 Rich-Harris