Skip to content

fix: add a warning when the misuse of reset in an error:boundary causes an error to be thrown when flushing the boundary content #16171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
5 changes: 5 additions & 0 deletions .changeset/new-dogs-obey.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: handle error in correct boundary after reset
5 changes: 5 additions & 0 deletions .changeset/polite-toys-report.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: make `<svelte:boundary>` reset function a noop after the first call
20 changes: 20 additions & 0 deletions documentation/docs/98-reference/.generated/client-errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,23 @@ let odd = $derived(!even);
```

If side-effects are unavoidable, use [`$effect`]($effect) instead.

### svelte_boundary_reset_onerror

```
A `<svelte:boundary>` `reset` function cannot be called while an error is still being handled
```

If a [`<svelte:boundary>`](https://svelte.dev/docs/svelte/svelte-boundary) has an `onerror` function, it must not call the provided `reset` function synchronously since the boundary is still in a broken state. Typically, `reset()` is called later, once the error has been resolved.

If it's possible to resolve the error inside the `onerror` callback, you must at least wait for the boundary to settle before calling `reset()`, for example using [`tick`](https://svelte.dev/docs/svelte/lifecycle-hooks#tick):

```svelte
<svelte:boundary onerror={async (error, reset) => {
fixTheError();
+++await tick();+++
reset();
}}>

</svelte:boundary>
```
26 changes: 26 additions & 0 deletions documentation/docs/98-reference/.generated/client-warnings.md
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,32 @@ Reactive `$state(...)` proxies and the values they proxy have different identiti

To resolve this, ensure you're comparing values where both values were created with `$state(...)`, or neither were. Note that `$state.raw(...)` will _not_ create a state proxy.

### svelte_boundary_reset_noop

```
A `<svelte:boundary>` `reset` function only resets the boundary the first time it is called
```

When an error occurs while rendering the contents of a [`<svelte:boundary>`](https://svelte.dev/docs/svelte/svelte-boundary), the `onerror` handler is called with the error plus a `reset` function that attempts to re-render the contents.

This `reset` function should only be called once. After that, it has no effect — in a case like this, where a reference to `reset` is stored outside the boundary, clicking the button while `<Contents />` is rendered will _not_ cause the contents to be rendered again.

```svelte
<script>
let reset;
</script>

<button onclick={reset}>reset</button>

<svelte:boundary onerror={(e, r) => (reset = r)}>
<!-- contents >

{#snippet failed(e)}
<p>oops! {e.message}</p>
{/snippet}
</svelte:boundary>
```

### transition_slide_display

```
Expand Down
18 changes: 18 additions & 0 deletions packages/svelte/messages/client-errors/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,3 +114,21 @@ let odd = $derived(!even);
```

If side-effects are unavoidable, use [`$effect`]($effect) instead.

## svelte_boundary_reset_onerror

> A `<svelte:boundary>` `reset` function cannot be called while an error is still being handled

If a [`<svelte:boundary>`](https://svelte.dev/docs/svelte/svelte-boundary) has an `onerror` function, it must not call the provided `reset` function synchronously since the boundary is still in a broken state. Typically, `reset()` is called later, once the error has been resolved.

If it's possible to resolve the error inside the `onerror` callback, you must at least wait for the boundary to settle before calling `reset()`, for example using [`tick`](https://svelte.dev/docs/svelte/lifecycle-hooks#tick):

```svelte
<svelte:boundary onerror={async (error, reset) => {
fixTheError();
+++await tick();+++
reset();
}}>

</svelte:boundary>
```
24 changes: 24 additions & 0 deletions packages/svelte/messages/client-warnings/warnings.md
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,30 @@ To silence the warning, ensure that `value`:
To resolve this, ensure you're comparing values where both values were created with `$state(...)`, or neither were. Note that `$state.raw(...)` will _not_ create a state proxy.
## svelte_boundary_reset_noop
> A `<svelte:boundary>` `reset` function only resets the boundary the first time it is called
When an error occurs while rendering the contents of a [`<svelte:boundary>`](https://svelte.dev/docs/svelte/svelte-boundary), the `onerror` handler is called with the error plus a `reset` function that attempts to re-render the contents.
This `reset` function should only be called once. After that, it has no effect — in a case like this, where a reference to `reset` is stored outside the boundary, clicking the button while `<Contents />` is rendered will _not_ cause the contents to be rendered again.
```svelte
<script>
let reset;
</script>

<button onclick={reset}>reset</button>

<svelte:boundary onerror={(e, r) => (reset = r)}>
<!-- contents >

{#snippet failed(e)}
<p>oops! {e.message}</p>
{/snippet}
</svelte:boundary>
```
## transition_slide_display
> The `slide` transition does not work correctly for elements with `display: %value%`
Expand Down
46 changes: 36 additions & 10 deletions packages/svelte/src/internal/client/dom/blocks/boundary.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/** @import { Effect, TemplateNode, } from '#client' */

import { BOUNDARY_EFFECT, EFFECT_TRANSPARENT } from '#client/constants';
import { BOUNDARY_EFFECT, EFFECT_RAN, EFFECT_TRANSPARENT } from '#client/constants';
import { component_context, set_component_context } from '../../context.js';
import { invoke_error_boundary } from '../../error-handling.js';
import { handle_error, invoke_error_boundary } from '../../error-handling.js';
import { block, branch, destroy_effect, pause_effect } from '../../reactivity/effects.js';
import {
active_effect,
Expand All @@ -19,6 +19,8 @@ import {
set_hydrate_node
} from '../hydration.js';
import { queue_micro_task } from '../task.js';
import * as w from '../../warnings.js';
import * as e from '../../errors.js';

/**
* @param {Effect} boundary
Expand All @@ -35,6 +37,8 @@ function with_boundary(boundary, fn) {

try {
fn();
} catch (e) {
handle_error(e);
} finally {
set_active_effect(previous_effect);
set_active_reaction(previous_reaction);
Expand Down Expand Up @@ -73,7 +77,29 @@ export function boundary(node, props, boundary_fn) {
throw error;
}

if (boundary_effect) {
destroy_effect(boundary_effect);
} else if (hydrating) {
set_hydrate_node(hydrate_open);
next();
set_hydrate_node(remove_nodes());
}

var did_reset = false;
var calling_on_error = false;

var reset = () => {
if (did_reset) {
w.svelte_boundary_reset_noop();
return;
}

did_reset = true;

if (calling_on_error) {
e.svelte_boundary_reset_onerror();
}

pause_effect(boundary_effect);

with_boundary(boundary, () => {
Expand All @@ -86,19 +112,19 @@ export function boundary(node, props, boundary_fn) {

try {
set_active_reaction(null);
calling_on_error = true;
onerror?.(error, reset);
calling_on_error = false;
} catch (error) {
if ((boundary.f & EFFECT_RAN) !== 0) {
invoke_error_boundary(error, /** @type {Effect} */ (boundary.parent));
} else {
throw error;
}
} finally {
set_active_reaction(previous_reaction);
}

if (boundary_effect) {
destroy_effect(boundary_effect);
} else if (hydrating) {
set_hydrate_node(hydrate_open);
next();
set_hydrate_node(remove_nodes());
}

if (failed) {
// Render the `failed` snippet in a microtask
queue_micro_task(() => {
Expand Down
8 changes: 7 additions & 1 deletion packages/svelte/src/internal/client/error-handling.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,13 @@ export function invoke_error_boundary(error, effect) {
// @ts-expect-error
effect.fn(error);
return;
} catch {}
} catch (e) {
if (DEV && e instanceof Error) {
adjust_error(e, effect);
}

error = e;
}
}

effect = effect.parent;
Expand Down
15 changes: 15 additions & 0 deletions packages/svelte/src/internal/client/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,4 +319,19 @@ export function state_unsafe_mutation() {
} else {
throw new Error(`https://svelte.dev/e/state_unsafe_mutation`);
}
}

/**
* A `<svelte:boundary>` `reset` function cannot be called while an error is still being handled
* @returns {never}
*/
export function svelte_boundary_reset_onerror() {
if (DEV) {
const error = new Error(`svelte_boundary_reset_onerror\nA \`<svelte:boundary>\` \`reset\` function cannot be called while an error is still being handled\nhttps://svelte.dev/e/svelte_boundary_reset_onerror`);

error.name = 'Svelte error';
throw error;
} else {
throw new Error(`https://svelte.dev/e/svelte_boundary_reset_onerror`);
}
}
11 changes: 11 additions & 0 deletions packages/svelte/src/internal/client/warnings.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,17 @@ export function state_proxy_equality_mismatch(operator) {
}
}

/**
* A `<svelte:boundary>` `reset` function only resets the boundary the first time it is called
*/
export function svelte_boundary_reset_noop() {
if (DEV) {
console.warn(`%c[svelte] svelte_boundary_reset_noop\n%cA \`<svelte:boundary>\` \`reset\` function only resets the boundary the first time it is called\nhttps://svelte.dev/e/svelte_boundary_reset_noop`, bold, normal);
} else {
console.warn(`https://svelte.dev/e/svelte_boundary_reset_noop`);
}
}

/**
* The `slide` transition does not work correctly for elements with `display: %value%`
* @param {string} value
Expand Down
7 changes: 4 additions & 3 deletions packages/svelte/tests/runtime-legacy/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -450,10 +450,11 @@ async function run_test_variant(
'Expected component to unmount and leave nothing behind after it was destroyed'
);

// TODO: This seems useless, unhandledRejection is only triggered on the next task
// by which time the test has already finished and the next test resets it to null above
// uncaught errors like during template effects flush
if (unhandled_rejection) {
throw unhandled_rejection; // eslint-disable-line no-unsafe-finally
if (!config.expect_unhandled_rejections) {
throw unhandled_rejection; // eslint-disable-line no-unsafe-finally
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
test({ assert, target }) {
const btn = target.querySelector('button');

btn?.click();

assert.throws(flushSync, 'svelte_boundary_reset_onerror');

// boundary content empty; only button remains
assert.htmlEqual(target.innerHTML, `<button>trigger throw</button>`);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<script>
let must_throw = $state(false);

function throw_error() {
throw new Error("error on template render");
}
</script>

<svelte:boundary onerror={(_, reset) => reset()}>
{must_throw ? throw_error() : 'normal content'}

{#snippet failed()}
<div>err</div>
{/snippet}
</svelte:boundary>

<button onclick={() => must_throw = true}>trigger throw</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
html: `
normal content
<button>toggle</button>
`,

async test({ assert, target, warnings }) {
const [btn] = target.querySelectorAll('button');

flushSync(() => btn.click());
assert.htmlEqual(target.innerHTML, `<div>err</div><button>toggle</button>`);
assert.deepEqual(warnings, []);

flushSync(() => btn.click());
assert.htmlEqual(target.innerHTML, `normal content <button>toggle</button>`);
assert.deepEqual(warnings, []);

flushSync(() => btn.click());
assert.htmlEqual(target.innerHTML, `<div>err</div><button>toggle</button>`);

assert.deepEqual(warnings, [
'A `<svelte:boundary>` `reset` function only resets the boundary the first time it is called'
]);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<script>
let must_throw = $state(false);
let reset = $state(null);

function throw_error() {
throw new Error("error on template render");
}
</script>

<svelte:boundary onerror={console.error}>
<svelte:boundary onerror={(_, fn) => (reset = fn)}>
{must_throw ? throw_error() : 'normal content'}

{#snippet failed()}
<div>err</div>
{/snippet}
</svelte:boundary>
</svelte:boundary>

<button
onclick={() => {
must_throw = !must_throw;
if (reset) reset();
}}>
toggle
</button>
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { flushSync } from 'svelte';
import { test } from '../../test';

export default test({
test({ assert, target, warnings }) {
const [toggle] = target.querySelectorAll('button');

flushSync(() => toggle.click());
assert.htmlEqual(
target.innerHTML,
// TODO the synthetic stack shouldn't be part of the message here
`<button>toggle</button><p>yikes! in {expression} in undefined</p><button>reset</button>`
);

const [, reset] = target.querySelectorAll('button');
flushSync(() => reset.click());
assert.htmlEqual(
target.innerHTML,
`<button>toggle</button><p>yikes! in {expression} in undefined</p><button>reset</button>`
);

flushSync(() => toggle.click());

const [, reset2] = target.querySelectorAll('button');
flushSync(() => reset2.click());
assert.htmlEqual(target.innerHTML, `<button>toggle</button><p>hello!</p>`);
}
});
Loading