fix: better bind:this cleanup timing (#17885)

This removes the `queue_micro_task`-workaround we employed in
`bind:this` in favor of a search for the nearest component effect /
effect that is still getting destroyed, whichever comes first.

We used `queue_micro_task` mainly due to timing issues with components
wanting to access the bound property on teardown still, and when nulling
it out on cleanup of the bind-this-effect itself, that was too early.
The microtask is too late though in some cases, when accessing
properties of objects that are no longer there. The targeted
upwards-walk solves this while keeping the binding around as long as
needed.

For that I had to add a new `DESTROYING` flag. We _could_ have done it
without one and by deleting code in `props.js` where we don't do
`get(d)` when the prop derived is destroyed, but I wanted to keep that
because you could still run into an access error if you e.g. access the
property in a timeout.

Alternative to #17862

Author: dummdidumm

.changeset/vast-moles-burn.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: better `bind:this` cleanup timing

packages/svelte/src/internal/client/constants.js

@@ -29,6 +29,8 @@ export const INERT = 1 << 13;
 export const DESTROYED = 1 << 14;
 /** Set once a reaction has run for the first time */
 export const REACTION_RAN = 1 << 15;
+/** Effect is in the process of getting destroyed. Can be observed in child teardown functions */
+export const DESTROYING = 1 << 25;
 
 // Flags exclusive to effects
 /**

packages/svelte/src/internal/client/context.js

@@ -5,7 +5,7 @@ import { active_effect, active_reaction } from './runtime.js';
 import { create_user_effect } from './reactivity/effects.js';
 import { async_mode_flag, legacy_mode_flag } from '../flags/index.js';
 import { FILENAME } from '../../constants.js';
-import { BRANCH_EFFECT, REACTION_RAN } from './constants.js';
+import { BRANCH_EFFECT } from './constants.js';
 
 /** @type {ComponentContext | null} */
 export let component_context = null;
@@ -182,6 +182,7 @@ export function push(props, runes = false, fn) {
 		e: null,
 		s: props,
 		x: null,
+		r: /** @type {Effect} */ (active_effect),
 		l: legacy_mode_flag && !runes ? { s: null, u: null, $: [] } : null
 	};
 

packages/svelte/src/internal/client/dom/elements/bindings/this.js

@@ -1,7 +1,8 @@
-import { STATE_SYMBOL } from '#client/constants';
+/** @import { ComponentContext, Effect } from '#client' */
+import { DESTROYING, STATE_SYMBOL } from '#client/constants';
+import { component_context } from '../../../context.js';
 import { effect, render_effect } from '../../../reactivity/effects.js';
-import { untrack } from '../../../runtime.js';
-import { queue_micro_task } from '../../task.js';
+import { active_effect, untrack } from '../../../runtime.js';
 
 /**
  * @param {any} bound_value
@@ -23,6 +24,9 @@ function is_bound_this(bound_value, element_or_component) {
  * @returns {void}
  */
 export function bind_this(element_or_component = {}, update, get_value, get_parts) {
+	var component_effect = /** @type {ComponentContext} */ (component_context).r;
+	var parent = /** @type {Effect} */ (active_effect);
+
 	effect(() => {
 		/** @type {unknown[]} */
 		var old_parts;
@@ -48,12 +52,25 @@ export function bind_this(element_or_component = {}, update, get_value, get_part
 		});
 
 		return () => {
-			// We cannot use effects in the teardown phase, we we use a microtask instead.
-			queue_micro_task(() => {
+			// When the bind:this effect is destroyed, we go up the effect parent chain until we find the last parent effect that is destroyed,
+			// or the effect containing the component bind:this is in (whichever comes first). That way we can time the nulling of the binding
+			// as close to user/developer expectation as possible.
+			// TODO Svelte 6: Decide if we want to keep this logic or just always null the binding in the component effect's teardown
+			// (which would be simpler, but less intuitive in some cases, and breaks the `ondestroy-before-cleanup` test)
+			let p = parent;
+			while (p !== component_effect && p.parent !== null && p.parent.f & DESTROYING) {
+				p = p.parent;
+			}
+			const teardown = () => {
 				if (parts && is_bound_this(get_value(...parts), element_or_component)) {
 					update(null, ...parts);
 				}
-			});
+			};
+			const original_teardown = p.teardown;
+			p.teardown = () => {
+				teardown();
+				original_teardown?.();
+			};
 		};
 	});
 

packages/svelte/src/internal/client/reactivity/effects.js

@@ -34,7 +34,8 @@ import {
 	USER_EFFECT,
 	ASYNC,
 	CONNECTED,
-	MANAGED_EFFECT
+	MANAGED_EFFECT,
+	DESTROYING
 } from '#client/constants';
 import * as e from '../errors.js';
 import { DEV } from 'esm-env';
@@ -520,9 +521,9 @@ export function destroy_effect(effect, remove_dom = true) {
 		removed = true;
 	}
 
+	set_signal_status(effect, DESTROYING);
 	destroy_effect_children(effect, remove_dom && !removed);
 	remove_reactions(effect, 0);
-	set_signal_status(effect, DESTROYED);
 
 	var transitions = effect.nodes && effect.nodes.t;
 
@@ -534,6 +535,9 @@ export function destroy_effect(effect, remove_dom = true) {
 
 	execute_effect_teardown(effect);
 
+	effect.f ^= DESTROYING;
+	effect.f |= DESTROYED;
+
 	var parent = effect.parent;
 
 	// If the parent doesn't have any children, then skip this work altogether

packages/svelte/src/internal/client/reactivity/props.js

@@ -419,9 +419,7 @@ export function prop(props, key, flags, fallback) {
 
 			// special case — avoid recalculating the derived if we're in a
 			// teardown function and the prop was overridden locally, or the
-			// component was already destroyed (this latter part is necessary
-			// because `bind:this` can read props after the component has
-			// been destroyed. TODO simplify `bind:this`
+			// component was already destroyed (people could access props in a timeout)
 			if ((is_destroying_effect && overridden) || (parent_effect.f & DESTROYED) !== 0) {
 				return d.v;
 			}

packages/svelte/src/internal/client/types.d.ts

@@ -38,6 +38,12 @@ export type ComponentContext = {
 	 * @deprecated remove in 6.0
 	 */
 	x: Record<string, any> | null;
+	/**
+	 * The parent effect of this component
+	 * TODO 6.0 this is used to control `bind:this` timing that might change,
+	 * in which case we can remove this property
+	 */
+	r: Effect;
 	/**
 	 * legacy stuff
 	 * @deprecated remove in 6.0

packages/svelte/tests/runtime-runes/samples/bind-this-destroy-timing/Inner.svelte

@@ -0,0 +1,9 @@
+<script>
+	let { data } = $props();
+
+	const processed = $derived(data.toUpperCase());
+
+	export function getProcessed() {
+		return processed;
+	}
+</script>

packages/svelte/tests/runtime-runes/samples/bind-this-destroy-timing/_config.js

@@ -0,0 +1,18 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		const [btn] = target.querySelectorAll('button');
+
+		btn.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>clear</button>
+				<p></p>
+			`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/bind-this-destroy-timing/main.svelte

@@ -0,0 +1,19 @@
+<script>
+	import Inner from './Inner.svelte';
+
+	let value = $state('hello');
+
+	let innerComp = $state();
+
+	// Reads Inner's derived value from outside the {#if} block, keeping it
+	// connected in the reactive graph even after the branch is destroyed.
+	const externalView = $derived(innerComp?.getProcessed() ?? '');
+</script>
+
+{#if value}
+	{@const result = value}
+	<Inner data={result} bind:this={innerComp} />
+{/if}
+
+<button onclick={() => (value = undefined)}>clear</button>
+<p>{externalView}</p>