From ef206fe1b5807ef7617c1fa34a33dd75ec7c8665 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 11 Mar 2024 17:57:02 +0000 Subject: [PATCH] fix: improve derived output for ssr (#10757) * fix: improve derived output for ssr * ts * Update .changeset/rotten-rules-invite.md --------- Co-authored-by: Rich Harris --- .changeset/rotten-rules-invite.md | 5 + .../3-transform/client/visitors/global.js | 1 - .../3-transform/server/transform-server.js | 182 +++++++++++------- .../phases/3-transform/server/types.d.ts | 2 + packages/svelte/src/internal/server/index.js | 15 ++ .../samples/class-state-derived-3/_config.js | 5 + .../samples/class-state-derived-3/main.svelte | 17 ++ 7 files changed, 156 insertions(+), 71 deletions(-) create mode 100644 .changeset/rotten-rules-invite.md create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-derived-3/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-derived-3/main.svelte diff --git a/.changeset/rotten-rules-invite.md b/.changeset/rotten-rules-invite.md new file mode 100644 index 0000000000..b175da8d6c --- /dev/null +++ b/.changeset/rotten-rules-invite.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: use getters for derived class state fields, with memoisation diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js index 5b8910b0d6..5d1689cadc 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/global.js @@ -17,7 +17,6 @@ export const global_visitors = { // rewrite `this.#foo` as `this.#foo.v` inside a constructor if (node.property.type === 'PrivateIdentifier') { const field = state.private_state.get(node.property.name); - if (field) { return state.in_constructor ? b.member(node, b.id('v')) : b.call('$.get', node); } 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 e9c7878c74..8ff7d2c316 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 @@ -546,82 +546,112 @@ const javascript_visitors = { /** @type {import('./types').Visitors} */ const javascript_visitors_runes = { - ClassBody(node, { state, visit, next }) { - if (!state.analysis.runes) { - next(); - } - /** @type {import('estree').PropertyDefinition[]} */ - const deriveds = []; - /** @type {import('estree').MethodDefinition | null} */ - let constructor = null; - // Get the constructor + ClassBody(node, { state, visit }) { + /** @type {Map} */ + const public_derived = new Map(); + + /** @type {Map} */ + const private_derived = new Map(); + + /** @type {string[]} */ + const private_ids = []; + for (const definition of node.body) { - if (definition.type === 'MethodDefinition' && definition.kind === 'constructor') { - constructor = /** @type {import('estree').MethodDefinition} */ (visit(definition)); - } - } - // Move $derived() runes to the end of the body if there is a constructor - if (constructor !== null) { - const body = []; - for (const definition of node.body) { - if ( - definition.type === 'PropertyDefinition' && - (definition.key.type === 'Identifier' || definition.key.type === 'PrivateIdentifier') - ) { - const is_private = definition.key.type === 'PrivateIdentifier'; - - if (definition.value?.type === 'CallExpression') { - const rune = get_rune(definition.value, state.scope); - - if (rune === '$derived') { - deriveds.push(/** @type {import('estree').PropertyDefinition} */ (visit(definition))); - if (is_private) { - // Keep the private #name initializer if private, but remove initial value - body.push({ - ...definition, - value: null - }); - } - continue; + if ( + definition.type === 'PropertyDefinition' && + (definition.key.type === 'Identifier' || definition.key.type === 'PrivateIdentifier') + ) { + const { type, name } = definition.key; + + const is_private = type === 'PrivateIdentifier'; + if (is_private) private_ids.push(name); + + if (definition.value?.type === 'CallExpression') { + const rune = get_rune(definition.value, state.scope); + if (rune === '$derived' || rune === '$derived.by') { + /** @type {import('../../3-transform/client/types.js').StateField} */ + const field = { + kind: rune === '$derived.by' ? 'derived_call' : 'derived', + // @ts-expect-error this is set in the next pass + id: is_private ? definition.key : null + }; + + if (is_private) { + private_derived.set(name, field); + } else { + public_derived.set(name, field); } } } - if (definition.type !== 'MethodDefinition' || definition.kind !== 'constructor') { - body.push( - /** @type {import('estree').PropertyDefinition | import('estree').MethodDefinition | import('estree').StaticBlock} */ ( - visit(definition) - ) - ); - } } - if (deriveds.length > 0) { - body.push({ - ...constructor, - value: { - ...constructor.value, - body: b.block([ - ...constructor.value.body.body, - ...deriveds.map((d) => { - return b.stmt( - b.assignment( - '=', - b.member(b.this, d.key), - /** @type {import('estree').Expression} */ (d.value) - ) - ); - }) - ]) + } + + // each `foo = $derived()` needs a backing `#foo` field + for (const [name, field] of public_derived) { + let deconflicted = name; + while (private_ids.includes(deconflicted)) { + deconflicted = '_' + deconflicted; + } + + private_ids.push(deconflicted); + field.id = b.private_id(deconflicted); + } + + /** @type {Array} */ + const body = []; + + const child_state = { ...state, private_derived }; + + // Replace parts of the class body + for (const definition of node.body) { + if ( + definition.type === 'PropertyDefinition' && + (definition.key.type === 'Identifier' || definition.key.type === 'PrivateIdentifier') + ) { + const name = definition.key.name; + + const is_private = definition.key.type === 'PrivateIdentifier'; + const field = (is_private ? private_derived : public_derived).get(name); + + if (definition.value?.type === 'CallExpression' && field !== undefined) { + const init = /** @type {import('estree').Expression} **/ ( + visit(definition.value.arguments[0], child_state) + ); + const value = + field.kind === 'derived_call' + ? b.call('$.once', init) + : b.call('$.once', b.thunk(init)); + + if (is_private) { + body.push(b.prop_def(field.id, value)); + } else { + // #foo; + const member = b.member(b.this, field.id); + body.push(b.prop_def(field.id, value)); + + // get foo() { return this.#foo; } + body.push(b.method('get', definition.key, [], [b.return(b.call(member))])); + + if ((field.kind === 'derived' || field.kind === 'derived_call') && state.options.dev) { + body.push( + b.method( + 'set', + definition.key, + [b.id('_')], + [b.throw_error(`Cannot update a derived property ('${name}')`)] + ) + ); + } } - }); - } else { - body.push(constructor); + + continue; + } } - return { - ...node, - body - }; + + body.push(/** @type {import('estree').MethodDefinition} **/ (visit(definition, child_state))); } - next(); + + return { ...node, body }; }, PropertyDefinition(node, { state, next, visit }) { if (node.value != null && node.value.type === 'CallExpression') { @@ -730,6 +760,16 @@ const javascript_visitors_runes = { return transform_inspect_rune(node, context); } + context.next(); + }, + MemberExpression(node, context) { + if (node.object.type === 'ThisExpression' && node.property.type === 'PrivateIdentifier') { + const field = context.state.private_derived.get(node.property.name); + if (field) { + return b.call(node); + } + } + context.next(); } }; @@ -2089,7 +2129,8 @@ export function server_component(analysis, options) { metadata: { namespace: options.namespace }, - preserve_whitespace: options.preserveWhitespace + preserve_whitespace: options.preserveWhitespace, + private_derived: new Map() }; const module = /** @type {import('estree').Program} */ ( @@ -2346,7 +2387,8 @@ export function server_module(analysis, options) { // this is an anomaly — it can only be used in components, but it needs // to be present for `javascript_visitors` and so is included in module // transform state as well as component transform state - legacy_reactive_statements: new Map() + legacy_reactive_statements: new Map(), + private_derived: new Map() }; const module = /** @type {import('estree').Program} */ ( 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 7f38932807..9cba57cd78 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 @@ -8,6 +8,7 @@ import type { import type { SvelteNode, Namespace, ValidatedCompileOptions } from '#compiler'; import type { TransformState } from '../types.js'; import type { ComponentAnalysis } from '../../types.js'; +import type { StateField } from '../client/types.js'; export type TemplateExpression = { type: 'expression'; @@ -35,6 +36,7 @@ export interface Anchor { export interface ServerTransformState extends TransformState { /** The $: calls, which will be ordered in the end */ readonly legacy_reactive_statements: Map; + readonly private_derived: Map; } export interface ComponentServerTransformState extends ServerTransformState { diff --git a/packages/svelte/src/internal/server/index.js b/packages/svelte/src/internal/server/index.js index c5fb7f1229..a6b0001d33 100644 --- a/packages/svelte/src/internal/server/index.js +++ b/packages/svelte/src/internal/server/index.js @@ -8,6 +8,7 @@ import { is_tag_valid_with_parent } from '../../constants.js'; import { DEV } from 'esm-env'; +import { UNINITIALIZED } from '../client/constants.js'; export * from '../client/validate.js'; @@ -666,3 +667,17 @@ export function loop_guard(timeout) { export function inspect(args, inspect = console.log) { inspect('init', ...args); } + +/** + * @template V + * @param {() => V} get_value + */ +export function once(get_value) { + let value = /** @type {V} */ (UNINITIALIZED); + return () => { + if (value === UNINITIALIZED) { + value = get_value(); + } + return value; + }; +} diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/_config.js new file mode 100644 index 0000000000..6f56b4455b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/_config.js @@ -0,0 +1,5 @@ +import { test } from '../../test'; + +export default test({ + html: `

a\n1

b\n2

c\n4

` +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/main.svelte new file mode 100644 index 0000000000..930496d85b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-derived-3/main.svelte @@ -0,0 +1,17 @@ + + +

a {foo.a}

+

b {foo.b}

+

c {foo.c}