fix: run effects in pending snippets (#17719)

Boundaries are buggy: if the `pending` snippet contains state, changes
to that state [won't cause
updates](https://svelte.dev/playground/hello-world?version=5.51.2#H4sIAAAAAAAAE22SQW-DMAyF_0qUTSpoE92uFJh223Hate0hJWaNliZRYsoqxH9fQtJW6noD-33Pz4aRKnYAWtIPkFKTQVvJSQZcIPCcPtNOSHC0XI8UTyboQsHXE_VuTOGOIDHUdszBvXqrFYJCb0Mr11phsNmoDUpAYsFpeQTrSE3W25Uv-0bXqxaFVsT0bp8dmewhJ2PobNB7OSQcOrAWuKc-rT4IB8UgcP91dsvyVZRf_IvZK8tJ3VzoInXTiCuDvVVXlYkT5u50k9DtRYfZJd11XGq8FSlKAsPOre4V-uSPDhlC9hIE1fJ6GFXtekRvrlUrRftTjzF25J5q8jrN9xOqtXDwhw14RO7jc5bIzI-3-vihyp3358yeZmFlmpENTGD8CIu0GV_kU7U0TdxmfHBKGON3MqC4UN9ZPsVDBHzOe1bjuEzaad7230j_nyD8Ii3R9jBt_RsTchCK07Jj0sH0B6hNF6aqAgAA):

```svelte
<script>
	let resolvers = [];

	function push(value) {
		const deferred = Promise.withResolvers();
		resolvers.push(() => deferred.resolve(value));
		return deferred.promise;
	}

	function shift() {
		resolvers.shift()?.();
	}

	let count = $state(0);
</script>

<button onclick={() => count += 1}>
	increment
</button>

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

<svelte:boundary>
	<p>{await push('resolved')}</p>

	{#snippet pending()}
		<p>{count}</p>
	{/snippet}
</svelte:boundary>
```

The issue is that the boundary's `this.#effect` has the
`BOUNDARY_EFFECT` flag, and `this.#pending_effect` is a child thereof.
Instead, `this.#main_effect` should have the flag. (It turns out
`this.#failed_effect` _also_ needs the flag, because errors that occur
in a `failed` snippet cause the boundary to re-render in its `failed`
state, which I found somewhat confusing to be honest. Probably the right
choice though.)

I was able to simplify the code a bit, too.

~~(Actually now that I think about it do we need `this.#effect` at all?
Will check.)~~

### 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`

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
pull/17728/head
Rich Harris 6 months ago committed by GitHub
parent ff70ab1b76
commit 6557a0a591
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: run effects in pending snippets

@ -1,6 +1,5 @@
/** @import { Effect, Source, TemplateNode, } from '#client' */
import {
BLOCK_EFFECT,
BOUNDARY_EFFECT,
COMMENT_NODE,
DIRTY,
@ -53,7 +52,7 @@ import { set_signal_status } from '../../reactivity/status.js';
* }} BoundaryProps
*/
var flags = EFFECT_TRANSPARENT | EFFECT_PRESERVED | BOUNDARY_EFFECT;
var flags = EFFECT_TRANSPARENT | EFFECT_PRESERVED;
/**
* @param {TemplateNode} node
@ -98,15 +97,10 @@ export class Boundary {
/** @type {DocumentFragment | null} */
#offscreen_fragment = null;
/** @type {TemplateNode | null} */
#pending_anchor = null;
#local_pending_count = 0;
#pending_count = 0;
#pending_count_update_queued = false;
#is_creating_fallback = false;
/** @type {Set<Effect>} */
#dirty_effects = new Set();
@ -142,51 +136,31 @@ export class Boundary {
constructor(node, props, children) {
this.#anchor = node;
this.#props = props;
this.#children = children;
this.parent = /** @type {Effect} */ (active_effect).b;
this.#children = (anchor) => {
var effect = /** @type {Effect} */ (active_effect);
this.is_pending = !!this.#props.pending;
effect.b = this;
effect.f |= BOUNDARY_EFFECT;
this.#effect = block(() => {
/** @type {Effect} */ (active_effect).b = this;
children(anchor);
};
this.parent = /** @type {Effect} */ (active_effect).b;
this.#effect = block(() => {
if (hydrating) {
const comment = this.#hydrate_open;
const comment = /** @type {Comment} */ (this.#hydrate_open);
hydrate_next();
const server_rendered_pending =
/** @type {Comment} */ (comment).nodeType === COMMENT_NODE &&
/** @type {Comment} */ (comment).data === HYDRATION_START_ELSE;
if (server_rendered_pending) {
if (comment.data === HYDRATION_START_ELSE) {
this.#hydrate_pending_content();
} else {
this.#hydrate_resolved_content();
if (this.#pending_count === 0) {
this.is_pending = false;
}
}
} else {
var anchor = this.#get_anchor();
try {
this.#main_effect = branch(() => children(anchor));
} catch (error) {
this.error(error);
}
if (this.#pending_count > 0) {
this.#show_pending_snippet();
} else {
this.is_pending = false;
}
this.#render();
}
return () => {
this.#pending_anchor?.remove();
};
}, flags);
if (hydrating) {
@ -206,19 +180,24 @@ export class Boundary {
const pending = this.#props.pending;
if (!pending) return;
this.is_pending = true;
this.#pending_effect = branch(() => pending(this.#anchor));
queue_micro_task(() => {
var anchor = this.#get_anchor();
var fragment = (this.#offscreen_fragment = document.createDocumentFragment());
var anchor = create_text();
fragment.append(anchor);
this.#main_effect = this.#run(() => {
Batch.ensure();
return branch(() => this.#children(anchor));
});
if (this.#pending_count > 0) {
this.#show_pending_snippet();
} else {
if (this.#pending_count === 0) {
this.#anchor.before(fragment);
this.#offscreen_fragment = null;
pause_effect(/** @type {Effect} */ (this.#pending_effect), () => {
this.#pending_effect = null;
});
@ -228,17 +207,28 @@ export class Boundary {
});
}
#get_anchor() {
var anchor = this.#anchor;
#render() {
try {
this.is_pending = this.has_pending_snippet();
this.#pending_count = 0;
this.#local_pending_count = 0;
this.#main_effect = branch(() => {
this.#children(this.#anchor);
});
if (this.is_pending) {
this.#pending_anchor = create_text();
this.#anchor.before(this.#pending_anchor);
if (this.#pending_count > 0) {
var fragment = (this.#offscreen_fragment = document.createDocumentFragment());
move_effect(this.#main_effect, fragment);
anchor = this.#pending_anchor;
const pending = /** @type {(anchor: Node) => void} */ (this.#props.pending);
this.#pending_effect = branch(() => pending(this.#anchor));
} else {
this.is_pending = false;
}
} catch (error) {
this.error(error);
}
return anchor;
}
/**
@ -262,7 +252,8 @@ export class Boundary {
}
/**
* @param {() => Effect | null} fn
* @template T
* @param {() => T} fn
*/
#run(fn) {
var previous_effect = active_effect;
@ -285,20 +276,6 @@ export class Boundary {
}
}
#show_pending_snippet() {
const pending = /** @type {(anchor: Node) => void} */ (this.#props.pending);
if (this.#main_effect !== null) {
this.#offscreen_fragment = document.createDocumentFragment();
this.#offscreen_fragment.append(/** @type {TemplateNode} */ (this.#pending_anchor));
move_effect(this.#main_effect, this.#offscreen_fragment);
}
if (this.#pending_effect === null) {
this.#pending_effect = branch(() => pending(this.#anchor));
}
}
/**
* Updates the pending count associated with the currently visible pending snippet,
* if any, such that we can replace the snippet with content once work is done
@ -383,7 +360,7 @@ export class Boundary {
// If we have nothing to capture the error, or if we hit an error while
// rendering the fallback, re-throw for another boundary to handle
if (this.#is_creating_fallback || (!onerror && !failed)) {
if (!onerror && !failed) {
throw error;
}
@ -423,31 +400,18 @@ export class Boundary {
e.svelte_boundary_reset_onerror();
}
// If the failure happened while flushing effects, current_batch can be null
Batch.ensure();
this.#local_pending_count = 0;
if (this.#failed_effect !== null) {
pause_effect(this.#failed_effect, () => {
this.#failed_effect = null;
});
}
// we intentionally do not try to find the nearest pending boundary. If this boundary has one, we'll render it on reset
// but it would be really weird to show the parent's boundary on a child reset.
this.is_pending = this.has_pending_snippet();
this.#run(() => {
// If the failure happened while flushing effects, current_batch can be null
Batch.ensure();
this.#main_effect = this.#run(() => {
this.#is_creating_fallback = false;
return branch(() => this.#children(this.#anchor));
this.#render();
});
if (this.#pending_count > 0) {
this.#show_pending_snippet();
} else {
this.is_pending = false;
}
};
queue_micro_task(() => {
@ -462,10 +426,16 @@ export class Boundary {
if (failed) {
this.#failed_effect = this.#run(() => {
Batch.ensure();
this.#is_creating_fallback = true;
try {
return branch(() => {
// errors in `failed` snippets cause the boundary to error again
// TODO Svelte 6: revisit this decision, most likely better to go to parent boundary instead
var effect = /** @type {Effect} */ (active_effect);
effect.b = this;
effect.f |= BOUNDARY_EFFECT;
failed(
this.#anchor,
() => error,
@ -475,8 +445,6 @@ export class Boundary {
} catch (error) {
invoke_error_boundary(error, /** @type {Effect} */ (this.#effect.parent));
return null;
} finally {
this.#is_creating_fallback = false;
}
});
}

@ -293,16 +293,19 @@ export class Batch {
}
}
var parent = effect.parent;
effect = effect.next;
while (effect === null && parent !== null) {
if (parent === pending_boundary) {
while (effect !== null) {
if (effect === pending_boundary) {
pending_boundary = null;
}
effect = parent.next;
parent = parent.parent;
var next = effect.next;
if (next !== null) {
effect = next;
break;
}
effect = effect.parent;
}
}
}

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

@ -0,0 +1,31 @@
<script>
let resolvers = [];
function push(value) {
const deferred = Promise.withResolvers();
resolvers.push(() => deferred.resolve(value));
return deferred.promise;
}
function shift() {
resolvers.shift()?.();
}
let count = $state(0);
</script>
<button onclick={() => count += 1}>
increment
</button>
<button onclick={shift}>
shift
</button>
<svelte:boundary>
<p>{await push('resolved')}</p>
{#snippet pending()}
<p>{count}</p>
{/snippet}
</svelte:boundary>
Loading…
Cancel
Save