From d45c8f066222c1dc268529750d20242ea363262f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Fri, 3 May 2024 23:37:58 +0200 Subject: [PATCH] feat: allow state and derived declarations inside class constructors closes #11116 closes #11339 --- .changeset/funny-icons-flash.md | 5 + .../compiler/phases/2-analyze/validation.js | 3 +- .../phases/3-transform/client/types.d.ts | 5 +- .../phases/3-transform/client/utils.js | 26 +++ .../3-transform/client/visitors/global.js | 18 ++- .../client/visitors/javascript-runes.js | 153 ++++++++++++------ .../class-state-in-constructor/_config.js | 31 ++++ .../class-state-in-constructor/main.svelte | 24 +++ 8 files changed, 211 insertions(+), 54 deletions(-) create mode 100644 .changeset/funny-icons-flash.md create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/main.svelte diff --git a/.changeset/funny-icons-flash.md b/.changeset/funny-icons-flash.md new file mode 100644 index 000000000000..4cd7af34a6ec --- /dev/null +++ b/.changeset/funny-icons-flash.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +feat: allow state and derived declarations inside class constructors diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index c2c7dece3147..aa395b25463e 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -821,7 +821,8 @@ function validate_call_expression(node, scope, path) { ) { if (parent.type === 'VariableDeclarator') return; if (parent.type === 'PropertyDefinition' && !parent.static && !parent.computed) return; - e.state_invalid_placement(node, rune); + // TODO + // e.state_invalid_placement(node, rune); } if (rune === '$effect' || rune === '$effect.pre') { diff --git a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts index c9287554a95d..137862a3bbe9 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts @@ -3,7 +3,8 @@ import type { Statement, LabeledStatement, Identifier, - PrivateIdentifier + PrivateIdentifier, + Literal } from 'estree'; import type { Namespace, SvelteNode, ValidatedCompileOptions } from '#compiler'; import type { TransformState } from '../types.js'; @@ -66,6 +67,8 @@ export interface ComponentClientTransformState extends ClientTransformState { export interface StateField { kind: 'state' | 'frozen_state' | 'derived' | 'derived_call'; + public_id: Identifier | Literal | null; + declared_in_constructor: boolean; id: PrivateIdentifier; } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index cc15c0c2f971..7e7a17c633e0 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -11,6 +11,7 @@ import { PROPS_IS_RUNES, PROPS_IS_UPDATED } from '../../../../constants.js'; +import { get_rune } from '../../scope.js'; /** * @template {import('./types').ClientTransformState} State @@ -231,6 +232,31 @@ export function serialize_set_binding(node, context, fallback, options) { state.in_constructor ) { const public_state = context.state.public_state.get(assignee.property.name); + const rune = get_rune(node.right, context.state.scope); + + if ( + public_state && + (rune === '$state' || + rune === '$state.frozen' || + rune === '$derived' || + rune === '$derived.by') + ) { + const args = /** @type {import('estree').CallExpression} */ (node.right).arguments; + let value = + args.length === 0 + ? b.id('undefined') + : /** @type {import('estree').Expression} */ (visit(args[0])); + if (rune === '$state' || rune === '$state.frozen') { + if (should_proxy_or_freeze(value, state.scope)) { + value = b.call(rune === '$state' ? '$.proxy' : '$.freeze', value); + } + value = b.call('$.source', value); + } else { + value = b.call('$.derived', rune === '$derived.by' ? value : b.thunk(value)); + } + return b.assignment(node.operator, b.member(b.this, public_state.id), value); + } + const value = get_assignment_value(node, context); // See if we should wrap value in $.proxy if ( 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 811407b35b84..b33d8167a72d 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 @@ -12,18 +12,26 @@ export const global_visitors = { return serialize_get_binding(node, state); } }, - MemberExpression(node, { state, next }) { + MemberExpression(node, { state, next, path }) { if (node.object.type === 'ThisExpression') { - // rewrite `this.#foo` as `this.#foo.v` inside a constructor + const parent = path.at(-1); + const is_constructor_assignment = + parent?.type === 'AssignmentExpression' && parent.left === node; + + // 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); + if (state.in_constructor && is_constructor_assignment) { + return b.member(node, b.id('v')); + } else { + return b.call('$.get', node); + } } } - // rewrite `this.foo` as `this.#foo.v` inside a constructor - if (node.property.type === 'Identifier' && !node.computed) { + // rewrite `this.foo = ...` as `this.#foo.v = ...` inside a constructor + if (node.property.type === 'Identifier' && !node.computed && is_constructor_assignment) { const field = state.public_state.get(node.property.name); if (field && state.in_constructor) { diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js index 9f3192c638c7..f748a6bbedb8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js @@ -50,6 +50,8 @@ export const javascript_visitors_runes = { : rune === '$derived.by' ? 'derived_call' : 'derived', + public_id: definition.key.type === 'PrivateIdentifier' ? null : definition.key, + declared_in_constructor: false, // @ts-expect-error this is set in the next pass id: is_private ? definition.key : null }; @@ -61,6 +63,61 @@ export const javascript_visitors_runes = { } } } + } else if (definition.type === 'MethodDefinition' && definition.kind === 'constructor') { + for (const entry of definition.value.body.body) { + if ( + entry.type === 'ExpressionStatement' && + entry.expression.type === 'AssignmentExpression' + ) { + let { left, right } = entry.expression; + if ( + left.type !== 'MemberExpression' || + left.object.type !== 'ThisExpression' || + (left.property.type !== 'Identifier' && left.property.type !== 'PrivateIdentifier') + ) { + continue; + } + + const id = left.property; + const name = get_name(id); + if (!name) continue; + + const is_private = id.type === 'PrivateIdentifier'; + if (is_private) private_ids.push(name); + + if (right.type === 'CallExpression') { + const rune = get_rune(right, state.scope); + if ( + rune === '$state' || + rune === '$state.frozen' || + rune === '$derived' || + rune === '$derived.by' + ) { + /** @type {import('../types.js').StateField} */ + const field = { + kind: + rune === '$state' + ? 'state' + : rune === '$state.frozen' + ? 'frozen_state' + : rune === '$derived.by' + ? 'derived_call' + : 'derived', + public_id: id.type === 'PrivateIdentifier' ? null : id, + declared_in_constructor: true, + // @ts-expect-error this is set in the next pass + id: is_private ? id : null + }; + + if (is_private) { + private_state.set(name, field); + } else { + public_state.set(name, field); + } + } + } + } + } } } @@ -78,6 +135,53 @@ export const javascript_visitors_runes = { /** @type {Array} */ const body = []; + // create getters and setters for public fields + for (const [name, field] of public_state) { + const public_id = /** @type {import('estree').Identifier | import('estree').Literal} */ ( + field.public_id + ); + const member = b.member(b.this, field.id); + + if (field.declared_in_constructor) { + // #foo; + body.push(b.prop_def(field.id, undefined)); + } + + // get foo() { return this.#foo; } + body.push(b.method('get', public_id, [], [b.return(b.call('$.get', member))])); + + if (field.kind === 'state' || field.kind === 'frozen_state') { + // set foo(value) { this.#foo = value; } + const value = b.id('value'); + body.push( + b.method( + 'set', + public_id, + [value], + [ + b.stmt( + b.call( + '$.set', + member, + b.call(field.kind === 'state' ? '$.proxy' : '$.freeze', value) + ) + ) + ] + ) + ); + } else if (state.options.dev) { + body.push( + b.method( + 'set', + public_id, + [b.id('_')], + [b.throw_error(`Cannot update a derived property ('${name}')`)] + ) + ); + } + } + + /** @type {import('../types.js').ComponentClientTransformState} */ const child_state = { ...state, public_state, private_state }; // Replace parts of the class body @@ -121,53 +225,8 @@ export const javascript_visitors_runes = { value = b.call('$.source'); } - 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('$.get', member))])); - - if (field.kind === 'state') { - // set foo(value) { this.#foo = value; } - const value = b.id('value'); - body.push( - b.method( - 'set', - definition.key, - [value], - [b.stmt(b.call('$.set', member, b.call('$.proxy', value)))] - ) - ); - } - - if (field.kind === 'frozen_state') { - // set foo(value) { this.#foo = value; } - const value = b.id('value'); - body.push( - b.method( - 'set', - definition.key, - [value], - [b.stmt(b.call('$.set', member, b.call('$.freeze', value)))] - ) - ); - } - - 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}')`)] - ) - ); - } - } + // #foo = $state/$derived(); + body.push(b.prop_def(field.id, value)); continue; } } diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/_config.js b/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/_config.js new file mode 100644 index 000000000000..d97406c2991c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/_config.js @@ -0,0 +1,31 @@ +import { test } from '../../test'; + +export default test({ + solo: true, + html: ` + + + `, + + async test({ assert, target }) { + const [btn1, btn2] = target.querySelectorAll('button'); + + await btn1?.click(); + assert.htmlEqual( + target.innerHTML, + ` + + + ` + ); + + await btn2?.click(); + assert.htmlEqual( + target.innerHTML, + ` + + + ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/main.svelte b/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/main.svelte new file mode 100644 index 000000000000..04f0bef182f9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/class-state-in-constructor/main.svelte @@ -0,0 +1,24 @@ + + + +