diff --git a/.changeset/early-tips-prove.md b/.changeset/early-tips-prove.md new file mode 100644 index 0000000000..2df03126d8 --- /dev/null +++ b/.changeset/early-tips-prove.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: invoke parent boundary of deriveds that throw diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js index ec1c650814..1480648ce5 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBoundary.js +++ b/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') { - // we need to delay the visit of the snippets in case they access a ConstTag that is declared - // after the snippets so that the visitor for the const tag can be updated - snippets_visits.push(() => { - /** @type {Statement[]} */ - 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') { + if (child.type === 'ConstTag') { + continue; + } + + if (child.type === 'SnippetBlock') { /** @type {Statement[]} */ - const init = []; - context.visit(child, { ...context.state, init }); - - if (dev) { - // In dev we must separate the declarations from the code - // that eagerly evaluate the expression... - for (const statement of init) { - if (statement.type === 'VariableDeclaration') { - external_statements.push(statement); - } else { - internal_statements.push(statement); - } - } - } else { - external_statements.push(...init); + const statements = []; + + context.visit(child, { ...context.state, init: statements }); + + const snippet = /** @type {VariableDeclaration} */ (statements[0]); + + const snippet_fn = dev + ? // @ts-expect-error we know this shape is correct + snippet.declarations[0].init.arguments[1] + : snippet.declarations[0].init; + + snippet_fn.body.body.unshift( + ...const_tags.filter((node) => node.type === 'VariableDeclaration') + ); + + 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(...internal_statements); - } + block.body.unshift(...const_tags); 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( - external_statements.length > 0 ? b.block([...external_statements, boundary]) : boundary - ); + context.state.init.push(hoisted.length > 0 ? b.block([...hoisted, boundary]) : boundary); } diff --git a/packages/svelte/src/internal/client/dom/blocks/boundary.js b/packages/svelte/src/internal/client/dom/blocks/boundary.js index 53060017b9..d7a53da1d1 100644 --- a/packages/svelte/src/internal/client/dom/blocks/boundary.js +++ b/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, - reset_is_throwing_error + set_active_reaction } 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) { diff --git a/packages/svelte/src/internal/client/error-handling.js b/packages/svelte/src/internal/client/error-handling.js new file mode 100644 index 0000000000..aeec1d8b47 --- /dev/null +++ b/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} */ +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 || ''}`; + 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') + }); + } +} diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 36be1ecd04..54994b9bd1 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/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', { - value: '{expression}' + // wrap the effect so that we can decorate stack trace with `in {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; } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index d790c0ad14..5920a05fc5 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/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'; - -// Used for DEV time error handling -/** @param {WeakSet} value */ -const handled_errors = new WeakSet(); -let is_throwing_error = false; +import { handle_error, invoke_error_boundary } from './error-handling.js'; 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) } } -/** - * @template V - * @param {Reaction} reaction - * @returns {V} - */ +/** @param {Reaction} reaction */ 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)) { - update_effect(effect); - - // 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 - // 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 - // immediate effects. - if (effect.deps === null && effect.first === null && effect.nodes_start === null) { - if (effect.teardown === null) { - // remove this effect from the graph - unlink_effect(effect); - } else { - // keep the effect in the graph, but free up some memory - effect.fn = null; - } + if (check_dirtiness(effect)) { + update_effect(effect); + + // 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 + // 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 + // immediate effects. + if (effect.deps === null && effect.first === null && effect.nodes_start === null) { + if (effect.teardown === null) { + // remove this effect from the graph + unlink_effect(effect); + } else { + // keep the effect in the graph, but free up some memory + 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)) { - update_effect(effect); - } - } catch (error) { - handle_error(error, effect, null, effect.ctx); + if (check_dirtiness(effect)) { + update_effect(effect); } } diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-12/_config.js b/packages/svelte/tests/runtime-runes/samples/error-boundary-12/_config.js index b3edfefa20..4e771dfafa 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-12/_config.js +++ b/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(); - flushSync(); - - assert.htmlEqual(target.innerHTML, `

Error occured

`); + assert.throws(() => { + flushSync(() => btn?.click()); + }, /kaboom/); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-12/main.svelte b/packages/svelte/tests/runtime-runes/samples/error-boundary-12/main.svelte index bae46c2590..d9dee1e2b0 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-12/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/error-boundary-12/main.svelte @@ -1,20 +1,18 @@ - {#each things as thing} -

{thing}

- {/each} + {d} {#snippet failed()}

Error occured

diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/Child.svelte b/packages/svelte/tests/runtime-runes/samples/error-boundary-13/Child.svelte index 6e607871d3..de482da985 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/Child.svelte +++ b/packages/svelte/tests/runtime-runes/samples/error-boundary-13/Child.svelte @@ -1,7 +1,14 @@ diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/_config.js b/packages/svelte/tests/runtime-runes/samples/error-boundary-13/_config.js index b3edfefa20..3825dc7811 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/_config.js +++ b/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, `

Error occured

`); + assert.htmlEqual(target.innerHTML, `

Error occurred

`); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/main.svelte b/packages/svelte/tests/runtime-runes/samples/error-boundary-13/main.svelte index 65b71106fa..56e4846675 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-13/main.svelte +++ b/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] - }) - + {#snippet failed()} -

Error occured

+

Error occurred

{/snippet}
diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-18/_config.js b/packages/svelte/tests/runtime-runes/samples/error-boundary-18/_config.js index fedaaa9ad1..e092d0e7c7 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-18/_config.js +++ b/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/); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/error-boundary-18/main.svelte b/packages/svelte/tests/runtime-runes/samples/error-boundary-18/main.svelte index b9de7126db..e73dcacc8a 100644 --- a/packages/svelte/tests/runtime-runes/samples/error-boundary-18/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/error-boundary-18/main.svelte @@ -1,16 +1,18 @@ { throw(e) }}> -
Count: {count}
- - {count} / {test} +
Count: {count}
+ + {count} / {maybe_throw()}