From 97d5cf178f98bb02d2d4d9329204819dc17fffc4 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 26 Jul 2024 04:11:28 -0400 Subject: [PATCH] chore: simplify assignments in server code (#12614) Also fixes an uncovered bug where store `+=/-=` etc assignments were not serialized correctly on the server --- .changeset/violet-otters-carry.md | 5 + .../phases/3-transform/client/utils.js | 2 +- .../server/visitors/AssignmentExpression.js | 115 ++++++++++++++- .../server/visitors/shared/utils.js | 135 +----------------- packages/svelte/src/internal/client/index.js | 2 +- .../src/internal/client/reactivity/store.js | 2 +- packages/svelte/src/internal/server/index.js | 2 +- .../samples/store-assignments/_config.js | 5 + .../samples/store-assignments/main.svelte | 11 ++ 9 files changed, 138 insertions(+), 141 deletions(-) create mode 100644 .changeset/violet-otters-carry.md create mode 100644 packages/svelte/tests/runtime-runes/samples/store-assignments/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte diff --git a/.changeset/violet-otters-carry.md b/.changeset/violet-otters-carry.md new file mode 100644 index 0000000000..553662c31f --- /dev/null +++ b/.changeset/violet-otters-carry.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: correctly update stores when reassigning with operator other than `=` 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 b68629cd4b..b9456f8653 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -382,7 +382,7 @@ export function serialize_set_binding(node, context, fallback, prefix, options) } return b.call( - '$.mutate_store', + '$.store_mutate', serialize_get_binding(b.id(left_name), state), b.assignment(node.operator, /** @type {Pattern}} */ (visit_node(node.left)), value), b.call('$.untrack', b.id('$' + left_name)) diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js index f22f67db73..3cee0a4eb8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js @@ -1,11 +1,118 @@ -/** @import { AssignmentExpression } from 'estree' */ -/** @import { Context } from '../types.js' */ -import { serialize_set_binding } from './shared/utils.js'; +/** @import { AssignmentExpression, AssignmentOperator, BinaryOperator, Expression, Node, Pattern } from 'estree' */ +/** @import { SvelteNode } from '#compiler' */ +/** @import { Context, ServerTransformState } from '../types.js' */ +import * as b from '../../../../utils/builders.js'; +import { extract_paths } from '../../../../utils/ast.js'; +import { serialize_get_binding } from './shared/utils.js'; /** * @param {AssignmentExpression} node * @param {Context} context */ export function AssignmentExpression(node, context) { - return serialize_set_binding(node, context, context.next); + const parent = /** @type {Node} */ (context.path.at(-1)); + const is_standalone = parent.type.endsWith('Statement'); + + if ( + node.left.type === 'ArrayPattern' || + node.left.type === 'ObjectPattern' || + node.left.type === 'RestElement' + ) { + const value = /** @type {Expression} */ (context.visit(node.right)); + const should_cache = value.type !== 'Identifier'; + const rhs = should_cache ? b.id('$$value') : value; + + let changed = false; + + const assignments = extract_paths(node.left).map((path) => { + const value = path.expression?.(rhs); + + let assignment = serialize_assignment('=', path.node, value, context); + if (assignment !== null) changed = true; + + return assignment ?? b.assignment('=', path.node, value); + }); + + if (!changed) { + // No change to output -> nothing to transform -> we can keep the original assignment + return context.next(); + } + + const sequence = b.sequence(assignments); + + if (!is_standalone) { + // this is part of an expression, we need the sequence to end with the value + sequence.expressions.push(rhs); + } + + if (should_cache) { + // the right hand side is a complex expression, wrap in an IIFE to cache it + return b.call(b.arrow([rhs], sequence), value); + } + + return sequence; + } + + return serialize_assignment(node.operator, node.left, node.right, context) || context.next(); +} + +/** + * Only returns an expression if this is not a `$store` assignment, as others can be kept as-is + * @param {AssignmentOperator} operator + * @param {Pattern} left + * @param {Expression} right + * @param {import('zimmerframe').Context} context + * @returns {Expression | null} + */ +function serialize_assignment(operator, left, right, context) { + let object = left; + + while (object.type === 'MemberExpression') { + // @ts-expect-error + object = object.object; + } + + if (object.type !== 'Identifier' || !is_store_name(object.name)) { + return null; + } + + const name = object.name.slice(1); + + if (!context.state.scope.get(name)) { + return null; + } + + if (object === left) { + let value = /** @type {Expression} */ (context.visit(right)); + + if (operator !== '=') { + // turn `x += 1` into `x = x + 1` + value = b.binary( + /** @type {BinaryOperator} */ (operator.slice(0, -1)), + serialize_get_binding(left, context.state), + value + ); + } + + return b.call('$.store_set', b.id(name), value); + } + + return b.call( + '$.store_mutate', + b.assignment('??=', b.id('$$store_subs'), b.object([])), + b.literal(object.name), + b.id(name), + b.assignment( + operator, + /** @type {Pattern} */ (context.visit(left)), + /** @type {Expression} */ (context.visit(right)) + ) + ); +} + +/** + * @param {string} name + */ +function is_store_name(name) { + return name[0] === '$' && /[A-Za-z_]/.test(name[1]); } 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 94f2bd52b7..f7dc979e4c 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 @@ -1,7 +1,7 @@ -/** @import { AssignmentExpression, AssignmentOperator, BinaryOperator, Expression, Identifier, Node, Pattern, Statement, TemplateElement } from 'estree' */ +/** @import { AssignmentOperator, Expression, Identifier, Node, Statement, TemplateElement } from 'estree' */ /** @import { Attribute, Comment, ExpressionTag, SvelteNode, Text } from '#compiler' */ /** @import { ComponentContext, ServerTransformState } from '../../types.js' */ -import { extract_paths } from '../../../../../utils/ast.js'; + import { escape_html } from '../../../../../../escaping.js'; import { BLOCK_CLOSE, @@ -227,134 +227,3 @@ export function serialize_get_binding(node, state) { return node; } - -/** - * @param {AssignmentExpression} node - * @param {import('zimmerframe').Context} context - * @param {() => any} fallback - * @returns {Expression} - */ -export function serialize_set_binding(node, context, fallback) { - const { state, visit } = context; - - if ( - node.left.type === 'ArrayPattern' || - node.left.type === 'ObjectPattern' || - node.left.type === 'RestElement' - ) { - // Turn assignment into an IIFE, so that `$.set` calls etc don't produce invalid code - const tmp_id = context.state.scope.generate('tmp'); - - /** @type {AssignmentExpression[]} */ - const original_assignments = []; - - /** @type {Expression[]} */ - const assignments = []; - - const paths = extract_paths(node.left); - - for (const path of paths) { - const value = path.expression?.(b.id(tmp_id)); - const assignment = b.assignment('=', path.node, value); - original_assignments.push(assignment); - assignments.push(serialize_set_binding(assignment, context, () => assignment)); - } - - if (assignments.every((assignment, i) => assignment === original_assignments[i])) { - // No change to output -> nothing to transform -> we can keep the original assignment - return fallback(); - } - - return b.call( - b.thunk( - b.block([ - b.const(tmp_id, /** @type {Expression} */ (visit(node.right))), - b.stmt(b.sequence(assignments)), - b.return(b.id(tmp_id)) - ]) - ) - ); - } - - if (node.left.type !== 'Identifier' && node.left.type !== 'MemberExpression') { - throw new Error(`Unexpected assignment type ${node.left.type}`); - } - - let left = node.left; - - while (left.type === 'MemberExpression') { - // @ts-expect-error - left = left.object; - } - - if (left.type !== 'Identifier') { - return fallback(); - } - - const is_store = is_store_name(left.name); - const left_name = is_store ? left.name.slice(1) : left.name; - const binding = state.scope.get(left_name); - - if (!binding) return fallback(); - - if (binding.mutation !== null) { - return binding.mutation(node, context); - } - - if ( - binding.kind !== 'state' && - binding.kind !== 'frozen_state' && - binding.kind !== 'prop' && - binding.kind !== 'bindable_prop' && - binding.kind !== 'each' && - binding.kind !== 'legacy_reactive' && - !is_store - ) { - // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? - return fallback(); - } - - const value = get_assignment_value(node, context); - if (left === node.left) { - if (is_store) { - return b.call('$.store_set', b.id(left_name), /** @type {Expression} */ (visit(node.right))); - } - return fallback(); - } else if (is_store) { - return b.call( - '$.mutate_store', - b.assignment('??=', b.id('$$store_subs'), b.object([])), - b.literal(left.name), - b.id(left_name), - b.assignment(node.operator, /** @type {Pattern} */ (visit(node.left)), value) - ); - } - return fallback(); -} - -/** - * @param {AssignmentExpression} node - * @param {Pick, 'visit' | 'state'>} context - */ -function get_assignment_value(node, { state, visit }) { - if (node.left.type === 'Identifier') { - const operator = node.operator; - return operator === '=' - ? /** @type {Expression} */ (visit(node.right)) - : // turn something like x += 1 into x = x + 1 - b.binary( - /** @type {BinaryOperator} */ (operator.slice(0, -1)), - serialize_get_binding(node.left, state), - /** @type {Expression} */ (visit(node.right)) - ); - } - - return /** @type {Expression} */ (visit(node.right)); -} - -/** - * @param {string} name - */ -function is_store_name(name) { - return name[0] === '$' && /[A-Za-z_]/.test(name[1]); -} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index edfa40cdd4..c70cb6674b 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -116,7 +116,7 @@ export { } from './reactivity/props.js'; export { invalidate_store, - mutate_store, + store_mutate, setup_stores, store_get, store_set, diff --git a/packages/svelte/src/internal/client/reactivity/store.js b/packages/svelte/src/internal/client/reactivity/store.js index 0f491fdaca..e28202aaf0 100644 --- a/packages/svelte/src/internal/client/reactivity/store.js +++ b/packages/svelte/src/internal/client/reactivity/store.js @@ -119,7 +119,7 @@ export function setup_stores() { * @param {V} new_value the new store value * @template V */ -export function mutate_store(store, expression, new_value) { +export function store_mutate(store, expression, new_value) { store.set(new_value); return expression; } diff --git a/packages/svelte/src/internal/server/index.js b/packages/svelte/src/internal/server/index.js index ee69621a88..b3ced7d828 100644 --- a/packages/svelte/src/internal/server/index.js +++ b/packages/svelte/src/internal/server/index.js @@ -362,7 +362,7 @@ export function store_set(store, value) { * @param {Store} store * @param {any} expression */ -export function mutate_store(store_values, store_name, store, expression) { +export function store_mutate(store_values, store_name, store, expression) { store_set(store, store_get(store_values, store_name, store)); return expression; } diff --git a/packages/svelte/tests/runtime-runes/samples/store-assignments/_config.js b/packages/svelte/tests/runtime-runes/samples/store-assignments/_config.js new file mode 100644 index 0000000000..59a0b48d7b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-assignments/_config.js @@ -0,0 +1,5 @@ +import { test } from '../../test'; + +export default test({ + html: '

3

' +}); diff --git a/packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte b/packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte new file mode 100644 index 0000000000..a77e6475a2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte @@ -0,0 +1,11 @@ + + +

{$count}