fix: invoke parent boundary of deriveds that throw (#16091)

* remove unused handle_error arguments

* move error handling code into separate module

* tidy up

* simplify

* remove unnecessary handle_error invocation

* WIP

* WIP

* WIP

* note to self

* WIP

* WIP

* unused

* WIP

* turns out we need this

* fix

* all tests passing

* unused

* unused

* tidy up

* unused

* unused

* tweak

* WIP

* tweak

* asked and answered

* tweak

* unused

* changeset

* lint

.changeset/early-tips-prove.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: invoke parent boundary of deriveds that throw

packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js

@@ -1,4 +1,4 @@
-/** @import { BlockStatement, Statement, Expression } from 'estree' */
+/** @import { BlockStatement, Statement, Expression, FunctionDeclaration, VariableDeclaration, ArrowFunctionExpression } from 'estree' */
 /** @import { AST } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
 import { dev } from '../../../../state.js';
@@ -34,62 +34,61 @@ export function SvelteBoundary(node, context) {
 	const nodes = [];
 
 	/** @type {Statement[]} */
-	const external_statements = [];
+	const const_tags = [];
 
 	/** @type {Statement[]} */
-	const internal_statements = [];
+	const hoisted = [];
 
-	const snippets_visits = [];
+	// const tags need to live inside the boundary, but might also be referenced in hoisted snippets.
+	// to resolve this we cheat: we duplicate const tags inside snippets
+	for (const child of node.fragment.nodes) {
+		if (child.type === 'ConstTag') {
+			context.visit(child, { ...context.state, init: const_tags });
+		}
+	}
 
-	// Capture the `failed` implicit snippet prop
 	for (const child of node.fragment.nodes) {
-		if (child.type === 'SnippetBlock' && child.expression.name === 'failed') {
+		if (child.type === 'ConstTag') {
-			// we need to delay the visit of the snippets in case they access a ConstTag that is declared
+			continue;
-			// after the snippets so that the visitor for the const tag can be updated
+		}
-			snippets_visits.push(() => {
+
-				/** @type {Statement[]} */
+		if (child.type === 'SnippetBlock') {
-				const init = [];
-				context.visit(child, { ...context.state, init });
-				props.properties.push(b.prop('init', child.expression, child.expression));
-				external_statements.push(...init);
-			});
-		} else if (child.type === 'ConstTag') {
 			/** @type {Statement[]} */
-			const init = [];
+			const statements = [];
-			context.visit(child, { ...context.state, init });
+
-
+			context.visit(child, { ...context.state, init: statements });
-			if (dev) {
+
-				// In dev we must separate the declarations from the code
+			const snippet = /** @type {VariableDeclaration} */ (statements[0]);
-				// that eagerly evaluate the expression...
+
-				for (const statement of init) {
+			const snippet_fn = dev
-					if (statement.type === 'VariableDeclaration') {
+				? // @ts-expect-error we know this shape is correct
-						external_statements.push(statement);
+					snippet.declarations[0].init.arguments[1]
-					} else {
+				: snippet.declarations[0].init;
-						internal_statements.push(statement);
+
-					}
+			snippet_fn.body.body.unshift(
-				}
+				...const_tags.filter((node) => node.type === 'VariableDeclaration')
-			} else {
+			);
-				external_statements.push(...init);
+
+			hoisted.push(snippet);
+
+			if (child.expression.name === 'failed') {
+				props.properties.push(b.prop('init', child.expression, child.expression));
 			}
-		} else {
+
-			nodes.push(child);
+			continue;
 		}
-	}
 
-	snippets_visits.forEach((visit) => visit());
+		nodes.push(child);
+	}
 
 	const block = /** @type {BlockStatement} */ (context.visit({ ...node.fragment, nodes }));
 
-	if (dev && internal_statements.length) {
+	block.body.unshift(...const_tags);
-		block.body.unshift(...internal_statements);
-	}
 
 	const boundary = b.stmt(
 		b.call('$.boundary', context.state.node, props, b.arrow([b.id('$$anchor')], block))
 	);
 
 	context.state.template.push_comment();
-	context.state.init.push(
+	context.state.init.push(hoisted.length > 0 ? b.block([...hoisted, boundary]) : boundary);
-		external_statements.length > 0 ? b.block([...external_statements, boundary]) : boundary
-	);
 }

packages/svelte/src/internal/client/dom/blocks/boundary.js

@@ -2,14 +2,13 @@
 
 import { BOUNDARY_EFFECT, EFFECT_TRANSPARENT } from '#client/constants';
 import { component_context, set_component_context } from '../../context.js';
+import { invoke_error_boundary } from '../../error-handling.js';
 import { block, branch, destroy_effect, pause_effect } from '../../reactivity/effects.js';
 import {
 	active_effect,
 	active_reaction,
-	handle_error,
 	set_active_effect,
-	set_active_reaction,
+	set_active_reaction
-	reset_is_throwing_error
 } from '../../runtime.js';
 import {
 	hydrate_next,
@@ -80,11 +79,17 @@ export function boundary(node, props, boundary_fn) {
 				with_boundary(boundary, () => {
 					is_creating_fallback = false;
 					boundary_effect = branch(() => boundary_fn(anchor));
-					reset_is_throwing_error();
 				});
 			};
 
-			onerror?.(error, reset);
+			var previous_reaction = active_reaction;
+
+			try {
+				set_active_reaction(null);
+				onerror?.(error, reset);
+			} finally {
+				set_active_reaction(previous_reaction);
+			}
 
 			if (boundary_effect) {
 				destroy_effect(boundary_effect);
@@ -109,10 +114,9 @@ export function boundary(node, props, boundary_fn) {
 								);
 							});
 						} catch (error) {
-							handle_error(error, boundary, null, boundary.ctx);
+							invoke_error_boundary(error, /** @type {Effect} */ (boundary.parent));
 						}
 
-						reset_is_throwing_error();
 						is_creating_fallback = false;
 					});
 				});
@@ -124,7 +128,6 @@ export function boundary(node, props, boundary_fn) {
 		}
 
 		boundary_effect = branch(() => boundary_fn(anchor));
-		reset_is_throwing_error();
 	}, EFFECT_TRANSPARENT | BOUNDARY_EFFECT);
 
 	if (hydrating) {

packages/svelte/src/internal/client/error-handling.js

@@ -0,0 +1,88 @@
+/** @import { Effect } from '#client' */
+import { DEV } from 'esm-env';
+import { FILENAME } from '../../constants.js';
+import { is_firefox } from './dom/operations.js';
+import { BOUNDARY_EFFECT, EFFECT_RAN } from './constants.js';
+import { define_property } from '../shared/utils.js';
+import { active_effect } from './runtime.js';
+
+/**
+ * @param {unknown} error
+ */
+export function handle_error(error) {
+	var effect = /** @type {Effect} */ (active_effect);
+
+	if (DEV && error instanceof Error) {
+		adjust_error(error, effect);
+	}
+
+	if ((effect.f & EFFECT_RAN) === 0) {
+		// if the error occurred while creating this subtree, we let it
+		// bubble up until it hits a boundary that can handle it
+		if ((effect.f & BOUNDARY_EFFECT) === 0) {
+			throw error;
+		}
+
+		// @ts-expect-error
+		effect.fn(error);
+	} else {
+		// otherwise we bubble up the effect tree ourselves
+		invoke_error_boundary(error, effect);
+	}
+}
+
+/**
+ * @param {unknown} error
+ * @param {Effect | null} effect
+ */
+export function invoke_error_boundary(error, effect) {
+	while (effect !== null) {
+		if ((effect.f & BOUNDARY_EFFECT) !== 0) {
+			try {
+				// @ts-expect-error
+				effect.fn(error);
+				return;
+			} catch {}
+		}
+
+		effect = effect.parent;
+	}
+
+	throw error;
+}
+
+/** @type {WeakSet<Error>} */
+const adjusted_errors = new WeakSet();
+
+/**
+ * Add useful information to the error message/stack in development
+ * @param {Error} error
+ * @param {Effect} effect
+ */
+function adjust_error(error, effect) {
+	if (adjusted_errors.has(error)) return;
+	adjusted_errors.add(error);
+
+	var indent = is_firefox ? '  ' : '\t';
+	var component_stack = `\n${indent}in ${effect.fn?.name || '<unknown>'}`;
+	var context = effect.ctx;
+
+	while (context !== null) {
+		component_stack += `\n${indent}in ${context.function?.[FILENAME].split('/').pop()}`;
+		context = context.p;
+	}
+
+	define_property(error, 'message', {
+		value: error.message + `\n${component_stack}\n`
+	});
+
+	if (error.stack) {
+		// Filter out internal modules
+		define_property(error, 'stack', {
+			value: error.stack
+				.split('\n')
+				.filter((line) => !line.includes('svelte/src/internal'))
+				.join('\n')
+		});
+	}
+}

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

@@ -332,16 +332,23 @@ export function render_effect(fn) {
  * @returns {Effect}
  */
 export function template_effect(fn, thunks = [], d = derived) {
-	const deriveds = thunks.map(d);
-	const effect = () => fn(...deriveds.map(get));
-
 	if (DEV) {
-		define_property(effect, 'name', {
+		// wrap the effect so that we can decorate stack trace with `in {expression}`
-			value: '{expression}'
+		// (TODO maybe there's a better approach?)
+		return render_effect(() => {
+			var outer = /** @type {Effect} */ (active_effect);
+			var inner = () => fn(...deriveds.map(get));
+
+			define_property(outer.fn, 'name', { value: '{expression}' });
+			define_property(inner, 'name', { value: '{expression}' });
+
+			const deriveds = thunks.map(d);
+			block(inner);
 		});
 	}
 
-	return block(effect);
+	const deriveds = thunks.map(d);
+	return block(() => fn(...deriveds.map(get)));
 }
 
 /**
@@ -426,7 +433,11 @@ export function destroy_block_effect_children(signal) {
 export function destroy_effect(effect, remove_dom = true) {
 	var removed = false;
 
-	if ((remove_dom || (effect.f & HEAD_EFFECT) !== 0) && effect.nodes_start !== null) {
+	if (
+		(remove_dom || (effect.f & HEAD_EFFECT) !== 0) &&
+		effect.nodes_start !== null &&
+		effect.nodes_end !== null
+	) {
 		remove_effect_dom(effect.nodes_start, /** @type {TemplateNode} */ (effect.nodes_end));
 		removed = true;
 	}

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

@@ -1,4 +1,4 @@
-/** @import { ComponentContext, Derived, Effect, Reaction, Signal, Source, Value } from '#client' */
+/** @import { Derived, Effect, Reaction, Signal, Source, Value } from '#client' */
 import { DEV } from 'esm-env';
 import { define_property, get_descriptors, get_prototype_of, index_of } from '../shared/utils.js';
 import {
@@ -22,14 +22,13 @@ import {
 	ROOT_EFFECT,
 	LEGACY_DERIVED_PROP,
 	DISCONNECTED,
-	BOUNDARY_EFFECT,
 	EFFECT_IS_UPDATING
 } from './constants.js';
 import { flush_tasks } from './dom/task.js';
 import { internal_set, old_values } from './reactivity/sources.js';
 import { destroy_derived_effects, update_derived } from './reactivity/deriveds.js';
 import * as e from './errors.js';
-import { FILENAME } from '../../constants.js';
+
 import { tracing_mode_flag } from '../flags/index.js';
 import { tracing_expressions, get_stack } from './dev/tracing.js';
 import {
@@ -39,12 +38,7 @@ import {
 	set_component_context,
 	set_dev_current_component_function
 } from './context.js';
-import { is_firefox } from './dom/operations.js';
+import { handle_error, invoke_error_boundary } from './error-handling.js';
-
-// Used for DEV time error handling
-/** @param {WeakSet<Error>} value */
-const handled_errors = new WeakSet();
-let is_throwing_error = false;
 
 let is_flushing = false;
 
@@ -227,131 +221,6 @@ export function check_dirtiness(reaction) {
 	return false;
 }
 
-/**
- * @param {unknown} error
- * @param {Effect} effect
- */
-function propagate_error(error, effect) {
-	/** @type {Effect | null} */
-	var current = effect;
-
-	while (current !== null) {
-		if ((current.f & BOUNDARY_EFFECT) !== 0) {
-			try {
-				// @ts-expect-error
-				current.fn(error);
-				return;
-			} catch {
-				// Remove boundary flag from effect
-				current.f ^= BOUNDARY_EFFECT;
-			}
-		}
-
-		current = current.parent;
-	}
-
-	is_throwing_error = false;
-	throw error;
-}
-
-/**
- * @param {Effect} effect
- */
-function should_rethrow_error(effect) {
-	return (
-		(effect.f & DESTROYED) === 0 &&
-		(effect.parent === null || (effect.parent.f & BOUNDARY_EFFECT) === 0)
-	);
-}
-
-export function reset_is_throwing_error() {
-	is_throwing_error = false;
-}
-
-/**
- * @param {unknown} error
- * @param {Effect} effect
- * @param {Effect | null} previous_effect
- * @param {ComponentContext | null} component_context
- */
-export function handle_error(error, effect, previous_effect, component_context) {
-	if (is_throwing_error) {
-		if (previous_effect === null) {
-			is_throwing_error = false;
-		}
-
-		if (should_rethrow_error(effect)) {
-			throw error;
-		}
-
-		return;
-	}
-
-	if (previous_effect !== null) {
-		is_throwing_error = true;
-	}
-
-	if (DEV && component_context !== null && error instanceof Error && !handled_errors.has(error)) {
-		handled_errors.add(error);
-
-		const component_stack = [];
-
-		const effect_name = effect.fn?.name;
-
-		if (effect_name) {
-			component_stack.push(effect_name);
-		}
-
-		/** @type {ComponentContext | null} */
-		let current_context = component_context;
-
-		while (current_context !== null) {
-			/** @type {string} */
-			var filename = current_context.function?.[FILENAME];
-
-			if (filename) {
-				const file = filename.split('/').pop();
-				component_stack.push(file);
-			}
-
-			current_context = current_context.p;
-		}
-
-		const indent = is_firefox ? '  ' : '\t';
-		define_property(error, 'message', {
-			value:
-				error.message + `\n${component_stack.map((name) => `\n${indent}in ${name}`).join('')}\n`
-		});
-		define_property(error, 'component_stack', {
-			value: component_stack
-		});
-
-		const stack = error.stack;
-
-		// Filter out internal files from callstack
-		if (stack) {
-			const lines = stack.split('\n');
-			const new_lines = [];
-			for (let i = 0; i < lines.length; i++) {
-				const line = lines[i];
-				if (line.includes('svelte/src/internal')) {
-					continue;
-				}
-				new_lines.push(line);
-			}
-			define_property(error, 'stack', {
-				value: new_lines.join('\n')
-			});
-		}
-	}
-
-	propagate_error(error, effect);
-
-	if (should_rethrow_error(effect)) {
-		throw error;
-	}
-}
-
 /**
  * @param {Value} signal
  * @param {Effect} effect
@@ -379,11 +248,7 @@ function schedule_possible_effect_self_invalidation(signal, effect, root = true)
 	}
 }
 
-/**
+/** @param {Reaction} reaction */
- * @template V
- * @param {Reaction} reaction
- * @returns {V}
- */
 export function update_reaction(reaction) {
 	var previous_deps = new_deps;
 	var previous_skipped_deps = skipped_deps;
@@ -473,6 +338,8 @@ export function update_reaction(reaction) {
 		}
 
 		return result;
+	} catch (error) {
+		handle_error(error);
 	} finally {
 		new_deps = previous_deps;
 		skipped_deps = previous_skipped_deps;
@@ -558,7 +425,6 @@ export function update_effect(effect) {
 	set_signal_status(effect, CLEAN);
 
 	var previous_effect = active_effect;
-	var previous_component_context = component_context;
 	var was_updating_effect = is_updating_effect;
 
 	active_effect = effect;
@@ -601,8 +467,6 @@ export function update_effect(effect) {
 		if (DEV) {
 			dev_effect_stack.push(effect);
 		}
-	} catch (error) {
-		handle_error(error, effect, previous_effect, previous_component_context || effect.ctx);
 	} finally {
 		is_updating_effect = was_updating_effect;
 		active_effect = previous_effect;
@@ -637,14 +501,14 @@ function infinite_loop_guard() {
 		if (last_scheduled_effect !== null) {
 			if (DEV) {
 				try {
-					handle_error(error, last_scheduled_effect, null, null);
+					invoke_error_boundary(error, last_scheduled_effect);
 				} catch (e) {
 					// Only log the effect stack if the error is re-thrown
 					log_effect_stack();
 					throw e;
 				}
 			} else {
-				handle_error(error, last_scheduled_effect, null, null);
+				invoke_error_boundary(error, last_scheduled_effect);
 			}
 		} else {
 			if (DEV) {
@@ -701,27 +565,23 @@ function flush_queued_effects(effects) {
 		var effect = effects[i];
 
 		if ((effect.f & (DESTROYED | INERT)) === 0) {
-			try {
+			if (check_dirtiness(effect)) {
-				if (check_dirtiness(effect)) {
+				update_effect(effect);
-					update_effect(effect);
+
-
+				// Effects with no dependencies or teardown do not get added to the effect tree.
-					// Effects with no dependencies or teardown do not get added to the effect tree.
+				// Deferred effects (e.g. `$effect(...)`) _are_ added to the tree because we
-					// Deferred effects (e.g. `$effect(...)`) _are_ added to the tree because we
+				// don't know if we need to keep them until they are executed. Doing the check
-					// don't know if we need to keep them until they are executed. Doing the check
+				// here (rather than in `update_effect`) allows us to skip the work for
-					// here (rather than in `update_effect`) allows us to skip the work for
+				// immediate effects.
-					// immediate effects.
+				if (effect.deps === null && effect.first === null && effect.nodes_start === null) {
-					if (effect.deps === null && effect.first === null && effect.nodes_start === null) {
+					if (effect.teardown === null) {
-						if (effect.teardown === null) {
+						// remove this effect from the graph
-							// remove this effect from the graph
+						unlink_effect(effect);
-							unlink_effect(effect);
+					} else {
-						} else {
+						// keep the effect in the graph, but free up some memory
-							// keep the effect in the graph, but free up some memory
+						effect.fn = null;
-							effect.fn = null;
-						}
 					}
 				}
-			} catch (error) {
-				handle_error(error, effect, null, effect.ctx);
 			}
 		}
 	}
@@ -780,12 +640,8 @@ function process_effects(root) {
 			} else if (is_branch) {
 				effect.f ^= CLEAN;
 			} else {
-				try {
+				if (check_dirtiness(effect)) {
-					if (check_dirtiness(effect)) {
+					update_effect(effect);
-						update_effect(effect);
-					}
-				} catch (error) {
-					handle_error(error, effect, null, effect.ctx);
 				}
 			}
 

packages/svelte/tests/runtime-runes/samples/error-boundary-12/_config.js

@@ -5,9 +5,8 @@ export default test({
 	test({ assert, target }) {
 		const btn = target.querySelector('button');
 
-		btn?.click();
+		assert.throws(() => {
-		flushSync();
+			flushSync(() => btn?.click());
-
+		}, /kaboom/);
-		assert.htmlEqual(target.innerHTML, `<button>change</button><p>Error occured</p>`);
 	}
 });

packages/svelte/tests/runtime-runes/samples/error-boundary-12/main.svelte

@@ -1,20 +1,18 @@
 <script>
 	let count = $state(0);
 
-	const things = $derived.by(() => {
+	const d = $derived.by(() => {
 		if (count === 1) {
-			throw new Error('123')
+			throw new Error('kaboom')
 		}
-		return [1, 2 ,3]
+		return count
 	})
 </script>
 
 <button onclick={() => count++}>change</button>
 
 <svelte:boundary>
-	{#each things as thing}
+	{d}
-		<p>{thing}</p>
-	{/each}
 
 	{#snippet failed()}
 		<p>Error occured</p>

packages/svelte/tests/runtime-runes/samples/error-boundary-13/Child.svelte

@@ -1,7 +1,14 @@
 <script>
-	const { things } = $props();
+	const { count } = $props();
+
+	const d = $derived.by(() => {
+		if (count === 1) {
+			throw new Error('kaboom')
+		}
+		return count
+	});
 
 	$effect(() => {
-		things
+		d;
-	})
+	});
 </script>

packages/svelte/tests/runtime-runes/samples/error-boundary-13/_config.js

@@ -8,6 +8,6 @@ export default test({
 		btn?.click();
 		flushSync();
 
-		assert.htmlEqual(target.innerHTML, `<button>change</button><p>Error occured</p>`);
+		assert.htmlEqual(target.innerHTML, `<button>change</button><p>Error occurred</p>`);
 	}
 });

packages/svelte/tests/runtime-runes/samples/error-boundary-13/main.svelte

@@ -2,21 +2,14 @@
 	import Child from './Child.svelte';
 
 	let count = $state(0);
-
-	const things = $derived.by(() => {
-		if (count === 1) {
-			throw new Error('123')
-		}
-		return [1, 2 ,3]
-	})
 </script>
 
 <button onclick={() => count++}>change</button>
 
 <svelte:boundary>
-	<Child {things} />
+	<Child {count} />
 
 	{#snippet failed()}
-		<p>Error occured</p>
+		<p>Error occurred</p>
 	{/snippet}
 </svelte:boundary>

packages/svelte/tests/runtime-runes/samples/error-boundary-18/_config.js

@@ -5,11 +5,11 @@ export default test({
 	test({ assert, target }) {
 		let btn = target.querySelector('button');
 
-		btn?.click();
-		btn?.click();
-
 		assert.throws(() => {
-			flushSync();
+			flushSync(() => {
+				btn?.click();
+				btn?.click();
+			});
 		}, /test\n\n\tin {expression}\n/);
 	}
 });

packages/svelte/tests/runtime-runes/samples/error-boundary-18/main.svelte

@@ -1,16 +1,18 @@
 <script>
 	let count = $state(0);
 
-	let test = $derived.by(() => {
+	function maybe_throw() {
 		if (count > 1) {
 			throw new Error('test');
 		}
-	});
+
+		return count;
+	}
 </script>
 
 <svelte:boundary onerror={(e) => { throw(e) }}>
-		<div>Count: {count}</div>
+	<div>Count: {count}</div>
-		<button onclick={() => count++}>Increment</button>
+	<button onclick={() => count++}>Increment</button>
-		{count} / {test}
+	{count} / {maybe_throw()}
 </svelte:boundary>