diff --git a/packages/svelte/src/internal/client/dom/blocks/portal.js b/packages/svelte/src/internal/client/dom/blocks/portal.js index f935e9338a..c0d73687b2 100644 --- a/packages/svelte/src/internal/client/dom/blocks/portal.js +++ b/packages/svelte/src/internal/client/dom/blocks/portal.js @@ -1,16 +1,11 @@ /** @import { Effect, Source, TemplateNode } from '#client' */ /** @import { Batch } from '../../reactivity/batch.js' */ import { DESTROYED, DESTROYING, HEAD_EFFECT } from '#client/constants'; +import { async_mode_flag } from '../../../flags/index.js'; import { capture } from '../../reactivity/async.js'; -import { current_batch } from '../../reactivity/batch.js'; -import { - block, - branch, - destroy_effect, - move_effect, - render_effect -} from '../../reactivity/effects.js'; -import { set, source } from '../../reactivity/sources.js'; +import { current_batch, eager_block_effects } from '../../reactivity/batch.js'; +import { block, branch, destroy_effect, move_effect } from '../../reactivity/effects.js'; +import { internal_set, source } from '../../reactivity/sources.js'; import { active_effect, get, untrack } from '../../runtime.js'; import { hydrate_next, @@ -80,28 +75,6 @@ function get_outlet_entry(key) { return entry; } -/** - * Run `fn` now (during hydration, where synchronous timing is required for - * claiming server-rendered content), or in a microtask otherwise. The latter - * ensures that outlet (un)registrations - which can happen while a batch is - * being committed - do not interfere with the commit by scheduling portal - * updates (which would happen in a new batch) at the wrong moment. - * @param {() => void} fn - */ -function run_outside_batch(fn) { - if (hydrating) { - fn(); - } else { - // TODO this is a hack to get around a (I think) general batch.js bug - // where setting state while flushing (render) effects can mess with - // #commit() of the earlier batch that runs afterwards, where roots - // would not be scheduled for other batches anymore because scheduling - // an effect might reach a branch that is already unclean, so scheduling - // thinks "oh we already have this root scheduled" (wrong because not in the context of that batch). - queue_micro_task(fn); - } -} - /** * Returns the node before which the content of a portal with the given * sequence number must be inserted, so that the contents of multiple portals @@ -159,54 +132,60 @@ export function portal_outlet(node, get_id) { if (hydrating) { // `node` is the `` comment — advance to the `` marker. // Server-rendered content of `{#portal ...}` blocks comes right after it - // and is claimed by the corresponding blocks during hydration + // and is claimed by the corresponding blocks during hydration. anchor = hydrate_next(); } /** @type {Outlet} */ var outlet = { anchor, claim: hydrating ? anchor : null, items: [] }; - // TODO this should be a block effect so it runs during traversal. The way it's right now - // it means that a #portal block with async work will have that async work not coordinated - // if it's instantiated through this @portal for the first time. - render_effect(() => { + // A block effect, so that (un)registration happens during batch traversal. + // Portals rendering into this outlet are scheduled and re-run within the + // same traversal (via `internal_set`, which schedules reactions into the + // current batch) - that way, any async work their content contains is + // discovered before the batch commits, and the batch waits for it. + block(() => { + const effect = /** @type {Effect} */ (active_effect); const id = get_id(); if (id == null) return; const entry = get_outlet_entry(id); - var registered = false; - var cancelled = false; + internal_set(entry.outlets, [...entry.outlets.v, outlet]); - const register = () => { - if (cancelled) return; - registered = true; - - set( - entry.outlets, - untrack(() => [...get(entry.outlets), outlet]) - ); - - // during hydration, portals that were created before this outlet claim - // their server-rendered content now, while the hydration position is known - for (const render of entry.pending) { - render(outlet); - } - }; + // During hydration, portals that were created before this outlet claim + // their server-rendered content now, while the hydration position is known. + for (const render of entry.pending) { + render(outlet); + } const unregister = () => { - cancelled = true; - if (!registered) return; - - set( + internal_set( entry.outlets, - untrack(() => get(entry.outlets).filter((o) => o !== outlet)) + entry.outlets.v.filter((o) => o !== outlet) ); }; - run_outside_batch(register); - - return () => run_outside_batch(unregister); + return () => { + if ((effect.f & (DESTROYED | DESTROYING)) !== 0) { + // The outlet is being destroyed, which happens while a batch is being + // committed. Unregistering right away would schedule the affected + // portals into a new batch mid-commit, interfering with how the + // committing batch reruns effects of other in-flight batches, so + // defer it (the DOM is already correct: portaled content is removed + // together with the outlet, or by the portals' own cleanup) + + // TODO this feels like a hack to get around a (I think) general batch.js bug + // where setting state while committing + flushing (render) effects can mess with + // rebase of the earlier batches in #commit() that runs afterwards, where roots + // would not be scheduled for other batches anymore because scheduling + // an effect might reach a branch that is already unclean, so scheduling + // thinks "oh we already have this root scheduled" (wrong because not in the context of that batch). + queue_micro_task(unregister); + } else { + unregister(); + } + }; }); if (hydrating) { @@ -434,7 +413,9 @@ export function portal(get_target, content) { // The outlets for this batch's key may have changed since the batch last // ran (an outlet can be (un)registered by another batch, without this // block necessarily re-running within this batch), so the target - // selection is computed from the now-committed state + // selection is computed from this batch's view of the world at commit + // time. The read must be batch-aware (`get` rather than `.v`), because + // other in-flight batches may have eagerly (un)registered outlets /** @type {Set} */ var targets = new Set(); @@ -442,7 +423,9 @@ export function portal(get_target, content) { if (target instanceof Element) { targets.add(target); } else { - for (var outlet of get_outlet_entry(target).outlets.v) { + var outlets = untrack(() => get(get_outlet_entry(target).outlets)); + + for (var outlet of outlets) { targets.add(outlet); } } @@ -485,7 +468,15 @@ export function portal(get_target, content) { var target = get_target(); var batch = /** @type {Batch} */ (current_batch); - var defer = should_defer_append(); + // Unlike other blocks (whose fresh content is always contained by the + // parent branch being created (at mount, or offscreen)) a portal renders + // its content outside its own subtree, potentially into DOM that is + // already visible. Insertion is therefore deferred until the batch + // commits even on the block's first run (which `should_defer_append` + // alone would treat as an immediate append), so that content never + // appears before the rest of the batch. + var defer = + should_defer_append() || (async_mode_flag && !hydrating && eager_block_effects === null); /** @type {Set} */ var targets = new Set(); @@ -528,7 +519,7 @@ export function portal(get_target, content) { // an outlet with our key may appear later during this hydration // pass (`{#portal ...}` before `{@portal ...}` in the markup). // Register a callback so it can have us claim our server-rendered - // content at its position + // content at its position. var entry = get_outlet_entry(target); var restore = capture(); diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 7d14b80519..cb493d805c 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -304,9 +304,6 @@ export class Batch { this.schedule(e); } - const roots = this.#roots; - this.#roots = []; - this.apply(); /** @type {Effect[]} */ @@ -321,20 +318,29 @@ export class Batch { */ var updates = (legacy_updates = []); - for (const root of roots) { - try { - this.#traverse(root, effects, render_effects); - } catch (e) { - reset_all(root); - // If there's no async work left, this branch is now dead and needs - // to be discarded to not become a zombie that is never cleaned up. - // See https://github.com/sveltejs/svelte/issues/18221#issuecomment-4497918414 - // for a (non-minimal) reproduction that demonstrates a case where this is necessary - // to not get follow-up false-positives via "batch has scheduled roots" invariant errors. - if (!this.#is_deferred()) this.discard(); - throw e; + do { + const roots = this.#roots; + this.#roots = []; + + for (const root of roots) { + try { + this.#traverse(root, effects, render_effects); + } catch (e) { + reset_all(root); + // If there's no async work left, this branch is now dead and needs + // to be discarded to not become a zombie that is never cleaned up. + // See https://github.com/sveltejs/svelte/issues/18221#issuecomment-4497918414 + // for a (non-minimal) reproduction that demonstrates a case where this is necessary + // to not get follow-up false-positives via "batch has scheduled roots" invariant errors. + if (!this.#is_deferred()) this.discard(); + throw e; + } } - } + + // If state was set during traversal, re-traverse right away to avoid + // tearing: e.g. an if block with async work inside could've been toggled, + // and without re-traversal we might wrongfully commit/render if there's no other async work left. + } while (this.#roots.length > 0); // any writes should take effect in a subsequent batch current_batch = null; diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/_config.js b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/_config.js new file mode 100644 index 0000000000..7eda1397ea --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/_config.js @@ -0,0 +1,16 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + const [toggle, resolve] = target.querySelectorAll('button'); + + toggle.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ` a`); + + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ` b b`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/main.svelte new file mode 100644 index 0000000000..64f425edb7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-1/main.svelte @@ -0,0 +1,23 @@ + + + + + +{#portal 'b'} + {await push('b')} +{/portal} + +{outletKey} +{#if outletKey === 'b'} + {@portal outletKey} +{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/_config.js b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/_config.js new file mode 100644 index 0000000000..7eda1397ea --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/_config.js @@ -0,0 +1,16 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + const [toggle, resolve] = target.querySelectorAll('button'); + + toggle.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ` a`); + + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ` b b`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/main.svelte new file mode 100644 index 0000000000..7bd2038479 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-2/main.svelte @@ -0,0 +1,21 @@ + + + + + +{@portal 'b'} + +{portal_key} +{#portal portal_key} + {await push('b')} +{/portal} diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/_config.js b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/_config.js new file mode 100644 index 0000000000..8d55bcc948 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/_config.js @@ -0,0 +1,21 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + const [show, resolve] = target.querySelectorAll('button'); + + show.click(); + await tick(); + // the batch is blocked on the portal content's async work — nothing + // (including the synchronous parts of the portal content) may appear yet + assert.htmlEqual(target.innerHTML, ` false`); + + resolve.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + `

static

async true` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/main.svelte new file mode 100644 index 0000000000..2b7d09ac68 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-portal-coordination-3/main.svelte @@ -0,0 +1,24 @@ + + + + + +{@portal 'target'} + +{show} +{#if show} + {#portal 'target'} +

static

+ {await push('async')} + {/portal} +{/if}