From 777c4dd3f87ccc34ec071f117c066136b1427f07 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 2 Mar 2025 20:17:14 +0100 Subject: [PATCH] fix: improve hydration comments for `$props.id` --- .changeset/brown-rocks-swim.md | 5 ++++ .../src/compiler/phases/2-analyze/index.js | 21 +++++++++++++---- .../src/compiler/phases/2-analyze/types.d.ts | 2 ++ .../utils/props_id_needs_hydration.js | 21 +++++++++++++++++ .../phases/2-analyze/visitors/AwaitBlock.js | 4 ++++ .../phases/2-analyze/visitors/KeyBlock.js | 4 ++++ .../phases/2-analyze/visitors/Text.js | 4 ++++ .../3-transform/client/transform-client.js | 5 +++- .../3-transform/server/transform-server.js | 15 ++++++++---- .../phases/3-transform/server/types.d.ts | 2 ++ .../3-transform/server/visitors/Fragment.js | 11 ++++++++- .../server/visitors/shared/utils.js | 23 +++++++++++++++++-- .../svelte/src/compiler/phases/types.d.ts | 1 + .../src/internal/client/dom/template.js | 8 +++++-- packages/svelte/src/internal/server/index.js | 8 +++++-- .../hydration/samples/props-id/Child1.svelte | 6 +++++ .../hydration/samples/props-id/Child2.svelte | 7 ++++++ .../hydration/samples/props-id/Child3.svelte | 8 +++++++ .../hydration/samples/props-id/_config.js | 17 ++++++++++++++ .../hydration/samples/props-id/_expected.html | 1 + .../hydration/samples/props-id/main.svelte | 11 +++++++++ 21 files changed, 166 insertions(+), 18 deletions(-) create mode 100644 .changeset/brown-rocks-swim.md create mode 100644 packages/svelte/src/compiler/phases/2-analyze/utils/props_id_needs_hydration.js create mode 100644 packages/svelte/tests/hydration/samples/props-id/Child1.svelte create mode 100644 packages/svelte/tests/hydration/samples/props-id/Child2.svelte create mode 100644 packages/svelte/tests/hydration/samples/props-id/Child3.svelte create mode 100644 packages/svelte/tests/hydration/samples/props-id/_config.js create mode 100644 packages/svelte/tests/hydration/samples/props-id/_expected.html create mode 100644 packages/svelte/tests/hydration/samples/props-id/main.svelte diff --git a/.changeset/brown-rocks-swim.md b/.changeset/brown-rocks-swim.md new file mode 100644 index 0000000000..d773080640 --- /dev/null +++ b/.changeset/brown-rocks-swim.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: improve hydration comments for `$props.id` diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 322293bf6b..776740576b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -250,7 +250,8 @@ export function analyze_module(ast, options) { accessors: false, runes: true, immutable: true, - tracing: false + tracing: false, + props_id_needs_hydration: true }; walk( @@ -269,7 +270,8 @@ export function analyze_module(ast, options) { has_props_rune: false, options: /** @type {ValidatedCompileOptions} */ (options), parent_element: null, - reactive_statement: null + reactive_statement: null, + props_id_needs_hydration: { value: true } }, visitors ); @@ -460,7 +462,8 @@ export function analyze_component(root, source, options) { source, undefined_exports: new Map(), snippet_renderers: new Map(), - snippets: new Set() + snippets: new Set(), + props_id_needs_hydration: true }; if (!runes) { @@ -618,10 +621,16 @@ export function analyze_component(root, source, options) { expression: null, derived_state: [], function_depth: scope.function_depth, - reactive_statement: null + reactive_statement: null, + // we need to use an object instead of the value because state is spread by the global visitor + props_id_needs_hydration: { value: true } }; walk(/** @type {AST.SvelteNode} */ (ast), state, visitors); + + if (!state.props_id_needs_hydration.value) { + analysis.props_id_needs_hydration = false; + } } // warn on any nonstate declarations that are a) reassigned and b) referenced in the template @@ -684,7 +693,9 @@ export function analyze_component(root, source, options) { component_slots: new Set(), expression: null, derived_state: [], - function_depth: scope.function_depth + function_depth: scope.function_depth, + // we need to use an object instead of the value because state is spread by the global visitor + props_id_needs_hydration: { value: true } }; walk(/** @type {AST.SvelteNode} */ (ast), state, visitors); diff --git a/packages/svelte/src/compiler/phases/2-analyze/types.d.ts b/packages/svelte/src/compiler/phases/2-analyze/types.d.ts index 17c8123de1..aefa5717e6 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/types.d.ts +++ b/packages/svelte/src/compiler/phases/2-analyze/types.d.ts @@ -20,6 +20,8 @@ export interface AnalysisState { expression: ExpressionMetadata | null; derived_state: { name: string; private: boolean }[]; function_depth: number; + // we need to use an object instead of the value because state is spread by the global visitor + props_id_needs_hydration: { value: boolean }; // legacy stuff reactive_statement: null | ReactiveStatement; diff --git a/packages/svelte/src/compiler/phases/2-analyze/utils/props_id_needs_hydration.js b/packages/svelte/src/compiler/phases/2-analyze/utils/props_id_needs_hydration.js new file mode 100644 index 0000000000..6de056d791 --- /dev/null +++ b/packages/svelte/src/compiler/phases/2-analyze/utils/props_id_needs_hydration.js @@ -0,0 +1,21 @@ +/** + * @param {import("#compiler").AST.BaseNode} node + * @param {import("../types").Context["path"]} path + */ +export function props_id_needs_hydration(node, path) { + return ( + path.length === 1 && + path[0].type === 'Fragment' && + path[0].nodes && + first_non_blank_text(path[0]) === node + ); +} + +/** + * @param {import("#compiler").AST.Fragment} fragment + */ +function first_non_blank_text(fragment) { + return fragment.nodes[0].type === 'Text' && fragment.nodes[0].data.trim() !== '' + ? fragment.nodes[0] + : fragment.nodes[1]; +} diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/AwaitBlock.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/AwaitBlock.js index a71f325154..5e9a79456f 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/AwaitBlock.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/AwaitBlock.js @@ -3,12 +3,16 @@ import { validate_block_not_empty, validate_opening_tag } from './shared/utils.js'; import * as e from '../../../errors.js'; import { mark_subtree_dynamic } from './shared/fragment.js'; +import { props_id_needs_hydration } from '../utils/props_id_needs_hydration.js'; /** * @param {AST.AwaitBlock} node * @param {Context} context */ export function AwaitBlock(node, context) { + if (props_id_needs_hydration(node, context.path)) { + context.state.props_id_needs_hydration.value = false; + } validate_block_not_empty(node.pending, context); validate_block_not_empty(node.then, context); validate_block_not_empty(node.catch, context); diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/KeyBlock.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/KeyBlock.js index 88bb6a98e7..a784711772 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/KeyBlock.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/KeyBlock.js @@ -1,5 +1,6 @@ /** @import { AST } from '#compiler' */ /** @import { Context } from '../types' */ +import { props_id_needs_hydration } from '../utils/props_id_needs_hydration.js'; import { mark_subtree_dynamic } from './shared/fragment.js'; import { validate_block_not_empty, validate_opening_tag } from './shared/utils.js'; @@ -8,6 +9,9 @@ import { validate_block_not_empty, validate_opening_tag } from './shared/utils.j * @param {Context} context */ export function KeyBlock(node, context) { + if (props_id_needs_hydration(node, context.path)) { + context.state.props_id_needs_hydration.value = false; + } validate_block_not_empty(node.fragment, context); if (context.state.analysis.runes) { diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js index 363a111b7d..5628869706 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Text.js @@ -3,12 +3,16 @@ import { is_tag_valid_with_parent } from '../../../../html-tree-validation.js'; import { regex_not_whitespace } from '../../patterns.js'; import * as e from '../../../errors.js'; +import { props_id_needs_hydration } from '../utils/props_id_needs_hydration.js'; /** * @param {AST.Text} node * @param {Context} context */ export function Text(node, context) { + if (props_id_needs_hydration(node, context.path)) { + context.state.props_id_needs_hydration.value = false; + } const in_template = context.path.at(-1)?.type === 'Fragment'; if (in_template && context.state.parent_element && regex_not_whitespace.test(node.data)) { diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index cf5ba285cb..30ef572978 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -564,7 +564,10 @@ export function client_component(analysis, options) { if (analysis.props_id) { // need to be placed on first line of the component for hydration - component_block.body.unshift(b.const(analysis.props_id, b.call('$.props_id'))); + component_block.body.unshift( + // if it needs hydration we need to also call next inside `props_id` + b.const(analysis.props_id, b.call('$.props_id', analysis.props_id_needs_hydration && b.true)) + ); } if (state.events.size > 0) { 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 df3d831d3c..4b6407e153 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 @@ -3,10 +3,10 @@ /** @import { ComponentServerTransformState, ComponentVisitors, ServerTransformState, Visitors } from './types.js' */ /** @import { Analysis, ComponentAnalysis } from '../../types.js' */ import { walk } from 'zimmerframe'; -import { set_scope } from '../../scope.js'; +import { dev, filename } from '../../../state.js'; import { extract_identifiers } from '../../../utils/ast.js'; import * as b from '../../../utils/builders.js'; -import { dev, filename } from '../../../state.js'; +import { set_scope } from '../../scope.js'; import { render_stylesheet } from '../css/index.js'; import { AssignmentExpression } from './visitors/AssignmentExpression.js'; import { AwaitBlock } from './visitors/AwaitBlock.js'; @@ -30,6 +30,7 @@ import { RenderTag } from './visitors/RenderTag.js'; import { SlotElement } from './visitors/SlotElement.js'; import { SnippetBlock } from './visitors/SnippetBlock.js'; import { SpreadAttribute } from './visitors/SpreadAttribute.js'; +import { SvelteBoundary } from './visitors/SvelteBoundary.js'; import { SvelteComponent } from './visitors/SvelteComponent.js'; import { SvelteElement } from './visitors/SvelteElement.js'; import { SvelteFragment } from './visitors/SvelteFragment.js'; @@ -38,7 +39,6 @@ import { SvelteSelf } from './visitors/SvelteSelf.js'; import { TitleElement } from './visitors/TitleElement.js'; import { UpdateExpression } from './visitors/UpdateExpression.js'; import { VariableDeclaration } from './visitors/VariableDeclaration.js'; -import { SvelteBoundary } from './visitors/SvelteBoundary.js'; /** @type {Visitors} */ const global_visitors = { @@ -100,7 +100,9 @@ export function server_component(analysis, options) { namespace: options.namespace, preserve_whitespace: options.preserveWhitespace, private_derived: new Map(), - skip_hydration_boundaries: false + skip_hydration_boundaries: false, + props_id: analysis.props_id?.name, + props_id_needs_hydration: analysis.props_id_needs_hydration }; const module = /** @type {Program} */ ( @@ -247,7 +249,10 @@ export function server_component(analysis, options) { if (analysis.props_id) { // need to be placed on first line of the component for hydration component_block.body.unshift( - b.const(analysis.props_id, b.call('$.props_id', b.id('$$payload'))) + b.const( + analysis.props_id, + b.call('$.props_id', b.id('$$payload'), analysis.props_id_needs_hydration && b.true) + ) ); } diff --git a/packages/svelte/src/compiler/phases/3-transform/server/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/server/types.d.ts index 971271642c..d416b1eceb 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/server/types.d.ts @@ -13,6 +13,8 @@ export interface ServerTransformState extends TransformState { export interface ComponentServerTransformState extends ServerTransformState { readonly analysis: ComponentAnalysis; readonly options: ValidatedCompileOptions; + readonly props_id?: string; + readonly props_id_needs_hydration: boolean; readonly init: Statement[]; 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 a293b98e7e..0565527cdb 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 @@ -42,5 +42,14 @@ export function Fragment(node, context) { process_children(trimmed, { ...context, state }); - return b.block([...state.init, ...build_template(state.template)]); + return b.block([ + ...state.init, + ...build_template( + state.template, + undefined, + undefined, + state.props_id, + state.props_id_needs_hydration + ) + ]); } 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 2c6aa2f316..390be54437 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 @@ -95,9 +95,17 @@ function is_statement(node) { * @param {Array} template * @param {Identifier} out * @param {AssignmentOperator} operator + * @param {string} [props_id] + * @param {boolean} [props_id_needs_hydration] * @returns {Statement[]} */ -export function build_template(template, out = b.id('$$payload.out'), operator = '+=') { +export function build_template( + template, + out = b.id('$$payload.out'), + operator = '+=', + props_id, + props_id_needs_hydration +) { /** @type {string[]} */ let strings = []; @@ -139,7 +147,18 @@ export function build_template(template, out = b.id('$$payload.out'), operator = } if (node.type === 'Literal') { - strings[strings.length - 1] += node.value; + if ( + i === 0 && + node.value === empty_comment.value && + !props_id_needs_hydration && + props_id != null + ) { + strings[strings.length - 1] += ``); + } else { + strings[strings.length - 1] += node.value; + } } else if (node.type === 'TemplateLiteral') { strings[strings.length - 1] += node.quasis[0].value.cooked; strings.push(...node.quasis.slice(1).map((q) => /** @type {string} */ (q.value.cooked))); diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index abe2b115de..c898bb08ac 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -28,6 +28,7 @@ export interface Analysis { runes: boolean; immutable: boolean; tracing: boolean; + props_id_needs_hydration: boolean; // TODO figure out if we can move this to ComponentAnalysis accessors: boolean; diff --git a/packages/svelte/src/internal/client/dom/template.js b/packages/svelte/src/internal/client/dom/template.js index 575bf55cf6..f0bac2d25d 100644 --- a/packages/svelte/src/internal/client/dom/template.js +++ b/packages/svelte/src/internal/client/dom/template.js @@ -258,8 +258,9 @@ export function reset_props_id() { /** * Create (or hydrate) an unique UID for the component instance. + * @param {boolean} needs_next */ -export function props_id() { +export function props_id(needs_next) { if ( hydrating && hydrate_node && @@ -267,7 +268,10 @@ export function props_id() { hydrate_node.textContent?.startsWith('#s') ) { const id = hydrate_node.textContent.substring(1); - hydrate_next(); + // in some cases we reuse the empty comment to store the id...in those cases we don't need to call next + if (needs_next) { + hydrate_next(); + } return id; } diff --git a/packages/svelte/src/internal/server/index.js b/packages/svelte/src/internal/server/index.js index 160a1faa65..1946a7a87b 100644 --- a/packages/svelte/src/internal/server/index.js +++ b/packages/svelte/src/internal/server/index.js @@ -536,11 +536,15 @@ export function once(get_value) { /** * Create an unique ID * @param {Payload} payload + * @param {boolean} [needs_hydration] * @returns {string} */ -export function props_id(payload) { +export function props_id(payload, needs_hydration) { const uid = payload.uid(); - payload.out += ''; + // we only add an hydration marker if the first comment of the component is not an empty comment + if (needs_hydration) { + payload.out += ''; + } return uid; } diff --git a/packages/svelte/tests/hydration/samples/props-id/Child1.svelte b/packages/svelte/tests/hydration/samples/props-id/Child1.svelte new file mode 100644 index 0000000000..a739c1defb --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/Child1.svelte @@ -0,0 +1,6 @@ + + +Text first +

{id}

\ No newline at end of file diff --git a/packages/svelte/tests/hydration/samples/props-id/Child2.svelte b/packages/svelte/tests/hydration/samples/props-id/Child2.svelte new file mode 100644 index 0000000000..750dff9f4c --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/Child2.svelte @@ -0,0 +1,7 @@ + + + +{#await Promise.resolve() then x}{/await} +

{id}

\ No newline at end of file diff --git a/packages/svelte/tests/hydration/samples/props-id/Child3.svelte b/packages/svelte/tests/hydration/samples/props-id/Child3.svelte new file mode 100644 index 0000000000..cf6ec5e0fe --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/Child3.svelte @@ -0,0 +1,8 @@ + + + +{#key id} +

{id}

+{/key} \ No newline at end of file diff --git a/packages/svelte/tests/hydration/samples/props-id/_config.js b/packages/svelte/tests/hydration/samples/props-id/_config.js new file mode 100644 index 0000000000..54c1ac97d1 --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/_config.js @@ -0,0 +1,17 @@ +import { test } from '../../test'; + +export default test({ + test(assert, target) { + assert.htmlEqual( + target.innerHTML, + ` +
+ Text first +

s1

+

s2

+

s3

+
+ ` + ); + } +}); diff --git a/packages/svelte/tests/hydration/samples/props-id/_expected.html b/packages/svelte/tests/hydration/samples/props-id/_expected.html new file mode 100644 index 0000000000..3fb31937de --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/_expected.html @@ -0,0 +1 @@ +
Text first

s1

s2

s3

diff --git a/packages/svelte/tests/hydration/samples/props-id/main.svelte b/packages/svelte/tests/hydration/samples/props-id/main.svelte new file mode 100644 index 0000000000..8046add003 --- /dev/null +++ b/packages/svelte/tests/hydration/samples/props-id/main.svelte @@ -0,0 +1,11 @@ + + +
+ + + +
\ No newline at end of file