chore: simplify assignments in server code (#12614)

Also fixes an uncovered bug where store `+=/-=` etc assignments were not serialized correctly on the server

Author: Rich-Harris

.changeset/violet-otters-carry.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: correctly update stores when reassigning with operator other than `=`

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))

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<SvelteNode, ServerTransformState>} 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]);
 }

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<SvelteNode, ServerTransformState>} 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<import('zimmerframe').Context<SvelteNode, ServerTransformState>, '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]);
-}

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,

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;
 }

packages/svelte/src/internal/server/index.js

@@ -362,7 +362,7 @@ export function store_set(store, value) {
  * @param {Store<V>} 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;
 }

packages/svelte/tests/runtime-runes/samples/store-assignments/_config.js

@@ -0,0 +1,5 @@
+import { test } from '../../test';
+
+export default test({
+	html: '<p>3</p>'
+});

packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte

@@ -0,0 +1,11 @@
+<script>
+	import { writable } from 'svelte/store';
+
+	const count = writable(0);
+
+	$count += 1;
+	$count += 1;
+	$count += 1;
+</script>
+
+<p>{$count}</p>