From aa9a72c17164cfac6c4a52e3e5d71e5bdba26245 Mon Sep 17 00:00:00 2001 From: "S. Elliott Johnson" Date: Thu, 11 Sep 2025 13:34:45 -0600 Subject: [PATCH] better errors for sync-in-async --- .../3-transform/server/transform-server.js | 19 ++---- .../3-transform/server/visitors/Fragment.js | 14 ++-- .../server/visitors/RegularElement.js | 4 +- .../server/visitors/SvelteBoundary.js | 3 +- .../server/visitors/TitleElement.js | 37 ++++------ .../server/visitors/shared/component.js | 13 +--- .../server/visitors/shared/utils.js | 2 +- .../svelte/src/compiler/phases/types.d.ts | 2 +- packages/svelte/src/internal/client/render.js | 5 +- .../svelte/src/internal/server/payload.js | 36 ++++++++-- .../src/internal/server/payload.test.ts | 67 ++++++++++++------- 11 files changed, 111 insertions(+), 91 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js index 3c83a13fa8..84f4779ba0 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/transform-server.js @@ -40,6 +40,7 @@ import { TitleElement } from './visitors/TitleElement.js'; import { UpdateExpression } from './visitors/UpdateExpression.js'; import { VariableDeclaration } from './visitors/VariableDeclaration.js'; import { SvelteBoundary } from './visitors/SvelteBoundary.js'; +import { call_child_payload } from './visitors/shared/utils.js'; /** @type {Visitors} */ const global_visitors = { @@ -238,18 +239,12 @@ export function server_component(analysis, options) { } const component_block = b.block([ - b.stmt( - b.call( - '$$payload.child', - b.arrow( - [b.id('$$payload')], - b.block([ - .../** @type {Statement[]} */ (instance.body), - .../** @type {Statement[]} */ (template.body) - ]), - analysis.suspends_without_fallback - ) - ) + call_child_payload( + b.block([ + .../** @type {Statement[]} */ (instance.body), + .../** @type {Statement[]} */ (template.body) + ]), + analysis.suspends_without_fallback ) ]); diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/Fragment.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/Fragment.js index 74d130e0db..427a111024 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/Fragment.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/Fragment.js @@ -2,7 +2,12 @@ /** @import { ComponentContext, ComponentServerTransformState } from '../types.js' */ import { clean_nodes, infer_namespace } from '../../utils.js'; import * as b from '#compiler/builders'; -import { empty_comment, process_children, build_template } from './shared/utils.js'; +import { + empty_comment, + process_children, + build_template, + call_child_payload +} from './shared/utils.js'; /** * @param {AST.Fragment} node @@ -49,12 +54,7 @@ export function Fragment(node, context) { b.array(node.metadata.hoisted_promises.promises) ), ...state.init, - b.stmt( - b.call( - '$$payload.child', - b.arrow([b.id('$$payload')], b.block(build_template(state.template)), true) - ) - ) + call_child_payload(b.block(build_template(state.template)), true) ]); } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/RegularElement.js index 111b43dfb5..1c9e76edbb 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/RegularElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/RegularElement.js @@ -12,7 +12,7 @@ import { process_children, build_template, build_attribute_value, - wrap_in_child_payload + call_child_payload } from './shared/utils.js'; /** @@ -203,7 +203,7 @@ export function RegularElement(node, context) { // TODO is this cast safe? const elements = state.template.splice(template_start, Infinity); state.template.push( - wrap_in_child_payload( + call_child_payload( b.block(build_template(elements)), // TODO this will always produce correct results (because it will produce an async function if the surrounding component is async) // but it will false-positive and create unnecessary async functions (eg. when the component is async but the select element is not) diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteBoundary.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteBoundary.js index 932983980c..69dbc5a530 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteBoundary.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/SvelteBoundary.js @@ -28,8 +28,7 @@ export function SvelteBoundary(node, context) { ); } } else { - // No pending snippet - render main content (may be async or sync) - context.state.template.push(b.literal(BLOCK_OPEN)); // + context.state.template.push(b.literal(BLOCK_OPEN)); context.state.template.push(/** @type {BlockStatement} */ (context.visit(node.fragment))); } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/TitleElement.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/TitleElement.js index 27bdb4ee9e..71e1cf4dae 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/TitleElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/TitleElement.js @@ -1,7 +1,7 @@ /** @import { AST } from '#compiler' */ /** @import { ComponentContext } from '../types.js' */ import * as b from '#compiler/builders'; -import { process_children, build_template } from './shared/utils.js'; +import { process_children, build_template, call_child_payload } from './shared/utils.js'; /** * @param {AST.TitleElement} node @@ -14,29 +14,20 @@ export function TitleElement(node, context) { template.push(b.literal('')); context.state.init.push( - b.stmt( - b.call( - '$$payload.child', - // this nonsense is necessary so that the write to the title is as tightly scoped to a specific location - // in the async tree as possible. This lets us use `get_path` to compare this assignment to other assignments - // so that we can overwrite earlier assignments with later ones. - b.arrow( - [b.id('$$payload')], - b.block([ - b.const('path', b.call('$$payload.get_path')), - b.let('title'), - ...build_template(template, b.id('title'), '='), - b.stmt( - b.assignment( - '=', - b.id('$$payload.global.head.title'), - b.object([b.init('path', b.id('path')), b.init('value', b.id('title'))]) - ) - ) - ]), - node.metadata.has_await + call_child_payload( + b.block([ + b.const('path', b.call('$$payload.get_path')), + b.let('title'), + ...build_template(template, b.id('title'), '='), + b.stmt( + b.assignment( + '=', + b.id('$$payload.global.head.title'), + b.object([b.init('path', b.id('path')), b.init('value', b.id('title'))]) + ) ) - ) + ]), + node.metadata.has_await ) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/component.js index da799a4d76..aa072bb09b 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/component.js @@ -1,7 +1,7 @@ /** @import { BlockStatement, Expression, Pattern, Property, SequenceExpression, Statement } from 'estree' */ /** @import { AST } from '#compiler' */ /** @import { ComponentContext } from '../../types.js' */ -import { empty_comment, build_attribute_value } from './utils.js'; +import { empty_comment, build_attribute_value, call_child_payload } from './utils.js'; import * as b from '#compiler/builders'; import { is_element_node } from '../../../../nodes.js'; import { dev } from '../../../../../state.js'; @@ -234,16 +234,7 @@ export function build_inline_component(node, expression, context) { // not necessary -- eg. when the component is asynchronous but the child content is not. // May or may not be worth optimizing. b.block([ - b.stmt( - b.call( - '$$payload.child', - b.arrow( - [b.id('$$payload')], - b.block(block.body), - context.state.analysis.suspends_without_fallback - ) - ) - ) + call_child_payload(b.block(block.body), context.state.analysis.suspends_without_fallback) ]) ); diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/utils.js index 78130b1e83..ccb42fa02a 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/utils.js @@ -263,6 +263,6 @@ export function build_getter(node, state) { * @param {boolean} async * @returns {Statement} */ -export function wrap_in_child_payload(body, async) { +export function call_child_payload(body, async) { return b.stmt(b.call('$$payload.child', b.arrow([b.id('$$payload')], body, async))); } diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index 5fd679337c..ffee91f646 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -107,7 +107,7 @@ export interface ComponentAnalysis extends Analysis { * Every snippet that is declared locally */ snippets: Set; - /** Whether the component uses `await` in a context that would require an `await` on the server. */ + /** Whether the component uses `await` in a context that causes suspense outside of any boundary with a pending snippet. */ suspends_without_fallback: boolean; hoisted_promises: Map; } diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index 52a32746a7..0910c6d06c 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -4,7 +4,6 @@ import { DEV } from 'esm-env'; import { clear_text_content, create_text, - first_child, get_first_child, get_next_sibling, init_operations @@ -12,7 +11,7 @@ import { import { HYDRATION_END, HYDRATION_ERROR, HYDRATION_START } from '../../constants.js'; import { active_effect } from './runtime.js'; import { push, pop, component_context } from './context.js'; -import { component_root, branch } from './reactivity/effects.js'; +import { component_root } from './reactivity/effects.js'; import { hydrate_next, hydrate_node, @@ -29,7 +28,7 @@ import { import { reset_head_anchor } from './dom/blocks/svelte-head.js'; import * as w from './warnings.js'; import * as e from './errors.js'; -import { append, assign_nodes, comment } from './dom/template.js'; +import { assign_nodes } from './dom/template.js'; import { is_passive_event } from '../../utils.js'; import { COMMENT_NODE } from './constants.js'; import { boundary } from './dom/blocks/boundary.js'; diff --git a/packages/svelte/src/internal/server/payload.js b/packages/svelte/src/internal/server/payload.js index a01f8a7786..c259bdf365 100644 --- a/packages/svelte/src/internal/server/payload.js +++ b/packages/svelte/src/internal/server/payload.js @@ -55,12 +55,12 @@ export class Payload { local; /** - * @param {TreeState} [global] + * @param {TreeState} global * @param {{ select_value: string | undefined }} [local] * @param {Payload | undefined} [parent] * @param {PayloadType} [type] */ - constructor(global = new TreeState(), local = { select_value: undefined }, parent, type) { + constructor(global, local = { select_value: undefined }, parent, type) { this.global = global; this.local = { ...local }; this.parent = parent; @@ -79,6 +79,12 @@ export class Payload { this.#out.push(child); const result = render(child); if (result instanceof Promise) { + if (this.global.mode === 'sync') { + // TODO more-proper error + throw new Error('Encountered an asynchronous component while rendering synchronously'); + } + // just to avoid unhandled promise rejections -- we'll end up throwing in `then` if something fails + result.catch(() => {}); child.promises.initial = result; } } @@ -135,8 +141,11 @@ export class Payload { collect() { const content = Payload.#collect_content(this.#out, this.type); if (content instanceof Promise) { - // TODO is there a good way to report where this is? Probably by using some sort of loc or stack trace in `child` creation. - throw new Error('Encountered an asynchronous component while rendering synchronously'); + // TODO improve message + // guess you could also end up here if you called `collect` in an async context but... just don't bro + throw new Error( + 'invariant: should never reach this, as child throws when it encounters async work in a synchronous context' + ); } return content; @@ -153,6 +162,11 @@ export class Payload { * @param {Payload} other */ subsume(other) { + if (this.global.mode !== other.global.mode) { + // TODO message - this should be impossible though + throw new Error('invariant: a payload cannot switch modes'); + } + this.global.subsume(other.global); this.local = other.local; this.#out = other.#out.map((item) => { @@ -287,6 +301,9 @@ export class TreeState { /** @type {TreeHeadState} */ #head; + /** @type {'sync' | 'async'} */ + #mode; + get css() { return this.#css; } @@ -299,17 +316,23 @@ export class TreeState { return this.#head; } + get mode() { + return this.#mode; + } + /** + * @param {'sync' | 'async'} mode * @param {string} [id_prefix] */ - constructor(id_prefix = '') { + constructor(mode, id_prefix = '') { this.#uid = props_id_generator(id_prefix); this.#css = new Set(); this.#head = new TreeHeadState(this.#uid); + this.#mode = mode; } copy() { - const state = new TreeState(); + const state = new TreeState(this.#mode); state.#css = new Set(this.#css); state.#head = this.#head.copy(); state.#uid = this.#uid; @@ -323,6 +346,7 @@ export class TreeState { this.#css = other.#css; this.#uid = other.#uid; this.#head.subsume(other.#head); + this.#mode = other.#mode; } } diff --git a/packages/svelte/src/internal/server/payload.test.ts b/packages/svelte/src/internal/server/payload.test.ts index 678afbcb3c..8407f0a4b0 100644 --- a/packages/svelte/src/internal/server/payload.test.ts +++ b/packages/svelte/src/internal/server/payload.test.ts @@ -2,7 +2,7 @@ import { assert, expect, test } from 'vitest'; import { Payload, TreeState, TreeHeadState } from './payload.js'; test('collects synchronous body content by default', () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('sync')); payload.push('a'); payload.child(($$payload) => { $$payload.push('b'); @@ -15,7 +15,7 @@ test('collects synchronous body content by default', () => { }); test('child type switches content area (head vs body)', () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('sync')); payload.push('a'); payload.child(($$payload) => { $$payload.push('T'); @@ -28,7 +28,7 @@ test('child type switches content area (head vs body)', () => { }); test('child inherits parent type when not specified', () => { - const parent = new Payload(undefined, undefined, undefined, 'head'); + const parent = new Payload(new TreeState('sync'), undefined, undefined, 'head'); parent.push(''); parent.child(($$payload) => { $$payload.push(''); @@ -39,7 +39,7 @@ test('child inherits parent type when not specified', () => { }); test('get_path returns the path indexes to a payload', () => { - const root = new Payload(); + const root = new Payload(new TreeState('sync')); let child_a: InstanceType | undefined; let child_b: InstanceType | undefined; let child_b_0: InstanceType | undefined; @@ -62,8 +62,19 @@ test('get_path returns the path indexes to a payload', () => { assert.deepEqual(child_b_0!.get_path(), [1, 0]); }); -test('awaiting payload resolves async children; collect throws on async', async () => { - const payload = new Payload(); +test('creating an async child in a sync context throws', () => { + const payload = new Payload(new TreeState('sync')); + payload.push('a'); + expect(() => + payload.child(async ($$payload) => { + await Promise.resolve(); + $$payload.push('x'); + }) + ).toThrow('Encountered an asynchronous component while rendering synchronously'); +}); + +test('awaiting payload resolves async children', async () => { + const payload = new Payload(new TreeState('async')); payload.push('a'); payload.child(async ($$payload) => { await Promise.resolve(); @@ -71,17 +82,13 @@ test('awaiting payload resolves async children; collect throws on async', async }); payload.push('y'); - expect(() => payload.collect()).toThrow( - 'Encountered an asynchronous component while rendering synchronously' - ); - const { body, head } = await payload; assert.equal(head, ''); assert.equal(body, 'axy'); }); test('then() allows awaiting payload to get aggregated content', async () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('async')); payload.push('1'); payload.child(async ($$payload) => { await Promise.resolve(); @@ -94,7 +101,7 @@ test('then() allows awaiting payload to get aggregated content', async () => { }); test('compact synchronously aggregates a range and can transform into head/body', () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('sync')); payload.push('a'); payload.push('b'); payload.push('c'); @@ -112,7 +119,7 @@ test('compact synchronously aggregates a range and can transform into head/body' }); test('compact schedules followup when compaction input is async', async () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('async')); payload.push('a'); payload.child(async ($$payload) => { await Promise.resolve(); @@ -131,7 +138,7 @@ test('compact schedules followup when compaction input is async', async () => { }); test('copy creates a deep copy of the tree and shares promises reference', () => { - const payload = new Payload(); + const payload = new Payload(new TreeState('sync')); let child_ref: InstanceType | undefined; payload.child(($$payload) => { child_ref = $$payload; @@ -154,7 +161,7 @@ test('copy creates a deep copy of the tree and shares promises reference', () => }); test('local state is shallow-copied to children', () => { - const root = new Payload(); + const root = new Payload(new TreeState('sync')); root.local.select_value = 'A'; let child: InstanceType | undefined; root.child(($$payload) => { @@ -167,11 +174,11 @@ test('local state is shallow-copied to children', () => { }); test('subsume replaces tree content and state from other', () => { - const a = new Payload(undefined, undefined, undefined, 'head'); + const a = new Payload(new TreeState('async'), undefined, undefined, 'head'); a.push(''); a.local.select_value = 'A'; - const b = new Payload(); + const b = new Payload(new TreeState('async')); b.child(async ($$payload) => { await Promise.resolve(); $$payload.push('body'); @@ -183,21 +190,35 @@ test('subsume replaces tree content and state from other', () => { a.subsume(b); - // content now matches b and is async - expect(() => a.collect()).toThrow( - 'Encountered an asynchronous component while rendering synchronously' - ); assert.equal(a.type, 'body'); assert.equal(a.local.select_value, 'B'); assert.strictEqual(a.promises, b.promises); // global state transferred - assert.ok(a.global.css.has({ hash: 'h', code: 'c' }) || [...a.global.css][0]?.hash === 'h'); + assert.ok([...a.global.css][0]?.hash === 'h'); assert.equal(a.global.head.title.value, 'Title'); }); +test('subsume refuses to switch modes', () => { + const a = new Payload(new TreeState('sync'), undefined, undefined, 'head'); + a.push(''); + a.local.select_value = 'A'; + + const b = new Payload(new TreeState('async')); + b.child(async ($$payload) => { + await Promise.resolve(); + $$payload.push('body'); + }); + b.global.css.add({ hash: 'h', code: 'c' }); + b.global.head.title = { path: [1], value: 'Title' }; + b.local.select_value = 'B'; + b.promises.initial = Promise.resolve(); + + expect(() => a.subsume(b)).toThrow('invariant: a payload cannot switch modes'); +}); + test('TreeState uid generator uses prefix and is shared by copy()', () => { - const state = new TreeState('id-'); + const state = new TreeState('sync', 'id-'); assert.equal(state.uid(), 'id-s1'); const state_copy = state.copy(); assert.equal(state_copy.uid(), 'id-s2');