From 8d3b17db6e3d7170c32345f7ed758ae8e20e5b9e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 15:04:54 -0400 Subject: [PATCH 01/15] remove some junk --- .../server/visitors/shared/utils.js | 43 ++++++------------- 1 file changed, 14 insertions(+), 29 deletions(-) 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 1d48019de65e..4dca75c6bb05 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 @@ -300,41 +300,26 @@ export function serialize_set_binding(node, context, fallback) { 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 - ) { + if (!binding || !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 b.call('$.store_set', b.id(left_name), /** @type {Expression} */ (visit(node.right))); } - return fallback(); + + 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)), + get_assignment_value(node, context) + ) + ); } /** From 497f639f4b6e3849b07a51f3bb8561202b37f916 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 15:30:26 -0400 Subject: [PATCH 02/15] tweak --- .../3-transform/server/visitors/shared/utils.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) 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 4dca75c6bb05..bc59ebb3870b 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 @@ -296,24 +296,26 @@ export function serialize_set_binding(node, context, fallback) { 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 (!is_store_name(left.name)) { + return fallback(); + } + + const name = left.name.slice(1); - if (!binding || !is_store) { + if (!state.scope.get(name)) { // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? return fallback(); } if (left === node.left) { - return b.call('$.store_set', b.id(left_name), /** @type {Expression} */ (visit(node.right))); + return b.call('$.store_set', b.id(name), /** @type {Expression} */ (visit(node.right))); } return b.call( '$.mutate_store', b.assignment('??=', b.id('$$store_subs'), b.object([])), b.literal(left.name), - b.id(left_name), + b.id(name), b.assignment( node.operator, /** @type {Pattern} */ (visit(node.left)), From fa6dd4da740d7f5f4487fe50c2be0cb8ef4c969f Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 17:11:14 -0400 Subject: [PATCH 03/15] simplify --- .../server/visitors/shared/utils.js | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) 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 bc59ebb3870b..22d54d273599 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 @@ -240,44 +240,43 @@ export function serialize_get_binding(node, state) { * @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 = []; + const rhs = b.id('$$value'); /** @type {Expression[]} */ const assignments = []; const paths = extract_paths(node.left); + let should_transform = false; + 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)); + const assignment = b.assignment('=', path.node, path.expression?.(rhs)); + let changed = true; + + assignments.push( + serialize_set_binding(assignment, context, () => { + changed = false; + return assignment; + }) + ); + + should_transform ||= changed; } - if (assignments.every((assignment, i) => assignment === original_assignments[i])) { + if (!should_transform) { // 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)) - ]) - ) + b.arrow([rhs], b.sequence([...assignments, rhs])), + /** @type {Expression} */ (context.visit(node.right)) ); } @@ -292,23 +291,19 @@ export function serialize_set_binding(node, context, fallback) { left = left.object; } - if (left.type !== 'Identifier') { - return fallback(); - } - - if (!is_store_name(left.name)) { + if (left.type !== 'Identifier' || !is_store_name(left.name)) { return fallback(); } const name = left.name.slice(1); - if (!state.scope.get(name)) { + if (!context.state.scope.get(name)) { // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? return fallback(); } if (left === node.left) { - return b.call('$.store_set', b.id(name), /** @type {Expression} */ (visit(node.right))); + return b.call('$.store_set', b.id(name), /** @type {Expression} */ (context.visit(node.right))); } return b.call( @@ -318,7 +313,7 @@ export function serialize_set_binding(node, context, fallback) { b.id(name), b.assignment( node.operator, - /** @type {Pattern} */ (visit(node.left)), + /** @type {Pattern} */ (context.visit(node.left)), get_assignment_value(node, context) ) ); From a16e0c105dd916e9113da634b20498c98146eba1 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 17:16:59 -0400 Subject: [PATCH 04/15] improve code generation --- .../server/visitors/shared/utils.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 22d54d273599..b3d83e1c6d6d 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 @@ -245,17 +245,16 @@ export function serialize_set_binding(node, context, fallback) { node.left.type === 'ObjectPattern' || node.left.type === 'RestElement' ) { - // Turn assignment into an IIFE, so that `$.set` calls etc don't produce invalid code - const rhs = b.id('$$value'); + const value = /** @type {Expression} */ (context.visit(node.right)); + + const should_cache = value.type !== 'Identifier'; + const rhs = should_cache ? b.id('$$value') : value; /** @type {Expression[]} */ const assignments = []; - - const paths = extract_paths(node.left); - let should_transform = false; - for (const path of paths) { + for (const path of extract_paths(node.left)) { const assignment = b.assignment('=', path.node, path.expression?.(rhs)); let changed = true; @@ -274,10 +273,11 @@ export function serialize_set_binding(node, context, fallback) { return fallback(); } - return b.call( - b.arrow([rhs], b.sequence([...assignments, rhs])), - /** @type {Expression} */ (context.visit(node.right)) - ); + if (should_cache) { + return b.call(b.arrow([rhs], b.sequence([...assignments, rhs])), value); + } else { + return b.call(b.thunk(b.sequence([...assignments, rhs]))); + } } if (node.left.type !== 'Identifier' && node.left.type !== 'MemberExpression') { From 44076f736c458be72ef11aa26f377931dcb28327 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 17:29:10 -0400 Subject: [PATCH 05/15] move code, improve generation --- .../server/visitors/AssignmentExpression.js | 134 +++++++++++++++++- .../server/visitors/shared/utils.js | 117 +-------------- 2 files changed, 132 insertions(+), 119 deletions(-) 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 f22f67db73d8..1d82d3e3026a 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,137 @@ -/** @import { AssignmentExpression } from 'estree' */ -/** @import { Context } from '../types.js' */ -import { serialize_set_binding } from './shared/utils.js'; +/** @import { AssignmentExpression, BinaryOperator, Expression, Node, Pattern } from 'estree' */ +/** @import { SvelteNode } from '#compiler' */ +/** @import { Context, ServerTransformState } from '../types.js' */ +import { extract_paths } from '../../../../utils/ast.js'; +import * as b from '../../../../utils/builders.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'); + return serialize_assignment(node, context, is_standalone, context.next); +} + +/** + * @param {AssignmentExpression} node + * @param {import('zimmerframe').Context} context + * @param {boolean} is_standalone + * @param {() => any} fallback + * @returns {Expression} + */ +function serialize_assignment(node, context, is_standalone, fallback) { + 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; + + /** @type {Expression[]} */ + const assignments = []; + let should_transform = false; + + for (const path of extract_paths(node.left)) { + const assignment = b.assignment('=', path.node, path.expression?.(rhs)); + let changed = true; + + assignments.push( + serialize_assignment(assignment, context, false, () => { + changed = false; + return assignment; + }) + ); + + should_transform ||= changed; + } + + if (!should_transform) { + // No change to output -> nothing to transform -> we can keep the original assignment + return fallback(); + } + + 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) { + return b.call(b.arrow([rhs], sequence), value); + } + + return sequence; + } + + 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' || !is_store_name(left.name)) { + return fallback(); + } + + const name = left.name.slice(1); + + if (!context.state.scope.get(name)) { + // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? + return fallback(); + } + + if (left === node.left) { + return b.call('$.store_set', b.id(name), /** @type {Expression} */ (context.visit(node.right))); + } + + return b.call( + '$.mutate_store', + b.assignment('??=', b.id('$$store_subs'), b.object([])), + b.literal(left.name), + b.id(name), + b.assignment( + node.operator, + /** @type {Pattern} */ (context.visit(node.left)), + get_assignment_value(node, context) + ) + ); +} + +/** + * @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/compiler/phases/3-transform/server/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/utils.js index b3d83e1c6d6d..7ace4984c64c 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, @@ -232,116 +232,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) { - 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; - - /** @type {Expression[]} */ - const assignments = []; - let should_transform = false; - - for (const path of extract_paths(node.left)) { - const assignment = b.assignment('=', path.node, path.expression?.(rhs)); - let changed = true; - - assignments.push( - serialize_set_binding(assignment, context, () => { - changed = false; - return assignment; - }) - ); - - should_transform ||= changed; - } - - if (!should_transform) { - // No change to output -> nothing to transform -> we can keep the original assignment - return fallback(); - } - - if (should_cache) { - return b.call(b.arrow([rhs], b.sequence([...assignments, rhs])), value); - } else { - return b.call(b.thunk(b.sequence([...assignments, rhs]))); - } - } - - 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' || !is_store_name(left.name)) { - return fallback(); - } - - const name = left.name.slice(1); - - if (!context.state.scope.get(name)) { - // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? - return fallback(); - } - - if (left === node.left) { - return b.call('$.store_set', b.id(name), /** @type {Expression} */ (context.visit(node.right))); - } - - return b.call( - '$.mutate_store', - b.assignment('??=', b.id('$$store_subs'), b.object([])), - b.literal(left.name), - b.id(name), - b.assignment( - node.operator, - /** @type {Pattern} */ (context.visit(node.left)), - get_assignment_value(node, context) - ) - ); -} - -/** - * @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]); -} From 20c9695e7befb3e1002e672eb43ff68edac43bed Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 17:31:40 -0400 Subject: [PATCH 06/15] simplify --- .../server/visitors/AssignmentExpression.js | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) 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 1d82d3e3026a..550f3ad18edd 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 @@ -29,29 +29,21 @@ function serialize_assignment(node, context, is_standalone, fallback) { 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; - /** @type {Expression[]} */ - const assignments = []; - let should_transform = false; + let unchanged = 0; - for (const path of extract_paths(node.left)) { + const assignments = extract_paths(node.left).map((path) => { const assignment = b.assignment('=', path.node, path.expression?.(rhs)); - let changed = true; - - assignments.push( - serialize_assignment(assignment, context, false, () => { - changed = false; - return assignment; - }) - ); - should_transform ||= changed; - } + return serialize_assignment(assignment, context, false, () => { + unchanged += 1; + return assignment; + }); + }); - if (!should_transform) { + if (unchanged === assignments.length) { // No change to output -> nothing to transform -> we can keep the original assignment return fallback(); } @@ -64,6 +56,7 @@ function serialize_assignment(node, context, is_standalone, fallback) { } 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); } From 5c818875b001d12af039309dd9c16083389ded5b Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 17:34:53 -0400 Subject: [PATCH 07/15] tidy up --- .../3-transform/server/visitors/AssignmentExpression.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 550f3ad18edd..68f5fcb378c4 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,8 +1,8 @@ /** @import { AssignmentExpression, BinaryOperator, Expression, Node, Pattern } from 'estree' */ /** @import { SvelteNode } from '#compiler' */ /** @import { Context, ServerTransformState } from '../types.js' */ -import { extract_paths } from '../../../../utils/ast.js'; import * as b from '../../../../utils/builders.js'; +import { extract_paths } from '../../../../utils/ast.js'; import { serialize_get_binding } from './shared/utils.js'; /** @@ -12,6 +12,7 @@ import { serialize_get_binding } from './shared/utils.js'; export function AssignmentExpression(node, context) { const parent = /** @type {Node} */ (context.path.at(-1)); const is_standalone = parent.type.endsWith('Statement'); + return serialize_assignment(node, context, is_standalone, context.next); } @@ -63,10 +64,6 @@ function serialize_assignment(node, context, is_standalone, fallback) { return sequence; } - 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') { From d093af00a593bf3101639cb8215475fcf01843d3 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:10:24 -0400 Subject: [PATCH 08/15] move some code --- .../server/visitors/AssignmentExpression.js | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) 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 68f5fcb378c4..0ae34fbece13 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 @@ -13,17 +13,6 @@ export function AssignmentExpression(node, context) { const parent = /** @type {Node} */ (context.path.at(-1)); const is_standalone = parent.type.endsWith('Statement'); - return serialize_assignment(node, context, is_standalone, context.next); -} - -/** - * @param {AssignmentExpression} node - * @param {import('zimmerframe').Context} context - * @param {boolean} is_standalone - * @param {() => any} fallback - * @returns {Expression} - */ -function serialize_assignment(node, context, is_standalone, fallback) { if ( node.left.type === 'ArrayPattern' || node.left.type === 'ObjectPattern' || @@ -38,7 +27,7 @@ function serialize_assignment(node, context, is_standalone, fallback) { const assignments = extract_paths(node.left).map((path) => { const assignment = b.assignment('=', path.node, path.expression?.(rhs)); - return serialize_assignment(assignment, context, false, () => { + return serialize_assignment(assignment, context, () => { unchanged += 1; return assignment; }); @@ -46,7 +35,7 @@ function serialize_assignment(node, context, is_standalone, fallback) { if (unchanged === assignments.length) { // No change to output -> nothing to transform -> we can keep the original assignment - return fallback(); + return context.next(); } const sequence = b.sequence(assignments); @@ -64,6 +53,16 @@ function serialize_assignment(node, context, is_standalone, fallback) { return sequence; } + return serialize_assignment(node, context, context.next); +} + +/** + * @param {AssignmentExpression} node + * @param {import('zimmerframe').Context} context + * @param {() => any} fallback + * @returns {Expression} + */ +function serialize_assignment(node, context, fallback) { let left = node.left; while (left.type === 'MemberExpression') { From d49f285e88685f5a826017abe2ef6671defaa389 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:13:34 -0400 Subject: [PATCH 09/15] unnecessary --- .../phases/3-transform/server/visitors/AssignmentExpression.js | 1 - 1 file changed, 1 deletion(-) 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 0ae34fbece13..0b3b12c2e118 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 @@ -77,7 +77,6 @@ function serialize_assignment(node, context, fallback) { const name = left.name.slice(1); if (!context.state.scope.get(name)) { - // TODO error if it's a computed (or rest prop)? or does that already happen elsewhere? return fallback(); } From bbdad4ba3f8da200d3e7a8e5f238af756dad83b5 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:23:15 -0400 Subject: [PATCH 10/15] fix --- .../server/visitors/AssignmentExpression.js | 34 +++++++------------ .../samples/store-assignments/_config.js | 5 +++ .../samples/store-assignments/main.svelte | 11 ++++++ 3 files changed, 28 insertions(+), 22 deletions(-) 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/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js index 0b3b12c2e118..63fb93d1d616 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 @@ -81,7 +81,17 @@ function serialize_assignment(node, context, fallback) { } if (left === node.left) { - return b.call('$.store_set', b.id(name), /** @type {Expression} */ (context.visit(node.right))); + const value = + node.operator === '=' + ? /** @type {Expression} */ (context.visit(node.right)) + : // turn something like x += 1 into x = x + 1 + b.binary( + /** @type {BinaryOperator} */ (node.operator.slice(0, -1)), + serialize_get_binding(node.left, context.state), + /** @type {Expression} */ (context.visit(node.right)) + ); + + return b.call('$.store_set', b.id(name), value); } return b.call( @@ -92,31 +102,11 @@ function serialize_assignment(node, context, fallback) { b.assignment( node.operator, /** @type {Pattern} */ (context.visit(node.left)), - get_assignment_value(node, context) + /** @type {Expression} */ (context.visit(node.right)) ) ); } -/** - * @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 */ 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 000000000000..59a0b48d7b62 --- /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 000000000000..a77e6475a20f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-assignments/main.svelte @@ -0,0 +1,11 @@ + + +

{$count}

From 3a90f07a8efa28e1884f064461013f8ec7db7b91 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:27:12 -0400 Subject: [PATCH 11/15] tweak --- .../server/visitors/AssignmentExpression.js | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) 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 63fb93d1d616..719b4b7078cf 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,4 +1,4 @@ -/** @import { AssignmentExpression, BinaryOperator, Expression, Node, Pattern } from 'estree' */ +/** @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'; @@ -25,9 +25,10 @@ export function AssignmentExpression(node, context) { let unchanged = 0; const assignments = extract_paths(node.left).map((path) => { - const assignment = b.assignment('=', path.node, path.expression?.(rhs)); + const value = path.expression?.(rhs); + const assignment = b.assignment('=', path.node, value); - return serialize_assignment(assignment, context, () => { + return serialize_assignment('=', path.node, value, context, () => { unchanged += 1; return assignment; }); @@ -53,42 +54,44 @@ export function AssignmentExpression(node, context) { return sequence; } - return serialize_assignment(node, context, context.next); + return serialize_assignment(node.operator, node.left, node.right, context, context.next); } /** - * @param {AssignmentExpression} node + * @param {AssignmentOperator} operator + * @param {Pattern} left + * @param {Expression} right * @param {import('zimmerframe').Context} context * @param {() => any} fallback * @returns {Expression} */ -function serialize_assignment(node, context, fallback) { - let left = node.left; +function serialize_assignment(operator, left, right, context, fallback) { + let object = left; - while (left.type === 'MemberExpression') { + while (object.type === 'MemberExpression') { // @ts-expect-error - left = left.object; + object = object.object; } - if (left.type !== 'Identifier' || !is_store_name(left.name)) { + if (object.type !== 'Identifier' || !is_store_name(object.name)) { return fallback(); } - const name = left.name.slice(1); + const name = object.name.slice(1); if (!context.state.scope.get(name)) { return fallback(); } - if (left === node.left) { + if (object === left) { const value = - node.operator === '=' - ? /** @type {Expression} */ (context.visit(node.right)) + operator === '=' + ? /** @type {Expression} */ (context.visit(right)) : // turn something like x += 1 into x = x + 1 b.binary( - /** @type {BinaryOperator} */ (node.operator.slice(0, -1)), - serialize_get_binding(node.left, context.state), - /** @type {Expression} */ (context.visit(node.right)) + /** @type {BinaryOperator} */ (operator.slice(0, -1)), + serialize_get_binding(left, context.state), + /** @type {Expression} */ (context.visit(right)) ); return b.call('$.store_set', b.id(name), value); @@ -97,12 +100,12 @@ function serialize_assignment(node, context, fallback) { return b.call( '$.mutate_store', b.assignment('??=', b.id('$$store_subs'), b.object([])), - b.literal(left.name), + b.literal(object.name), b.id(name), b.assignment( - node.operator, - /** @type {Pattern} */ (context.visit(node.left)), - /** @type {Expression} */ (context.visit(node.right)) + operator, + /** @type {Pattern} */ (context.visit(left)), + /** @type {Expression} */ (context.visit(right)) ) ); } From 84ed96197d8c28ed0d63353395e24b0b45e27255 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:32:57 -0400 Subject: [PATCH 12/15] tidy up --- .../server/visitors/AssignmentExpression.js | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) 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 719b4b7078cf..a938df5f0942 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 @@ -22,19 +22,18 @@ export function AssignmentExpression(node, context) { const should_cache = value.type !== 'Identifier'; const rhs = should_cache ? b.id('$$value') : value; - let unchanged = 0; + let changed = false; const assignments = extract_paths(node.left).map((path) => { const value = path.expression?.(rhs); - const assignment = b.assignment('=', path.node, value); - return serialize_assignment('=', path.node, value, context, () => { - unchanged += 1; - return assignment; - }); + let assignment = serialize_assignment('=', path.node, value, context); + if (assignment !== null) changed = true; + + return assignment ?? b.assignment('=', path.node, value); }); - if (unchanged === assignments.length) { + if (!changed) { // No change to output -> nothing to transform -> we can keep the original assignment return context.next(); } @@ -54,7 +53,7 @@ export function AssignmentExpression(node, context) { return sequence; } - return serialize_assignment(node.operator, node.left, node.right, context, context.next); + return serialize_assignment(node.operator, node.left, node.right, context) || context.next(); } /** @@ -62,10 +61,9 @@ export function AssignmentExpression(node, context) { * @param {Pattern} left * @param {Expression} right * @param {import('zimmerframe').Context} context - * @param {() => any} fallback - * @returns {Expression} + * @returns {Expression | null} */ -function serialize_assignment(operator, left, right, context, fallback) { +function serialize_assignment(operator, left, right, context) { let object = left; while (object.type === 'MemberExpression') { @@ -74,25 +72,26 @@ function serialize_assignment(operator, left, right, context, fallback) { } if (object.type !== 'Identifier' || !is_store_name(object.name)) { - return fallback(); + return null; } const name = object.name.slice(1); if (!context.state.scope.get(name)) { - return fallback(); + return null; } if (object === left) { - const value = - operator === '=' - ? /** @type {Expression} */ (context.visit(right)) - : // turn something like x += 1 into x = x + 1 - b.binary( - /** @type {BinaryOperator} */ (operator.slice(0, -1)), - serialize_get_binding(left, context.state), - /** @type {Expression} */ (context.visit(right)) - ); + 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); } From 897a091946c392f4ab96651731929bd269fc4bd0 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:34:01 -0400 Subject: [PATCH 13/15] changeset --- .changeset/violet-otters-carry.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/violet-otters-carry.md diff --git a/.changeset/violet-otters-carry.md b/.changeset/violet-otters-carry.md new file mode 100644 index 000000000000..553662c31fba --- /dev/null +++ b/.changeset/violet-otters-carry.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: correctly update stores when reassigning with operator other than `=` From f9aef712e1d631dfee3872f983dadb3b0414169a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 25 Jul 2024 18:34:50 -0400 Subject: [PATCH 14/15] more consistent naming --- packages/svelte/src/compiler/phases/3-transform/client/utils.js | 2 +- .../phases/3-transform/server/visitors/AssignmentExpression.js | 2 +- packages/svelte/src/internal/client/index.js | 2 +- packages/svelte/src/internal/client/reactivity/store.js | 2 +- packages/svelte/src/internal/server/index.js | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) 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 b68629cd4b9e..b9456f8653b7 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 a938df5f0942..bc4e8e104ec8 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 @@ -97,7 +97,7 @@ function serialize_assignment(operator, left, right, context) { } return b.call( - '$.mutate_store', + '$.store_mutate', b.assignment('??=', b.id('$$store_subs'), b.object([])), b.literal(object.name), b.id(name), diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index edfa40cdd435..c70cb6674b65 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 0f491fdacafd..e28202aaf096 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 a6d70d21de71..5f19a2c5acf7 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; } From df19c833c045cac8bc136c73cb7ee5fe8d919692 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Fri, 26 Jul 2024 10:10:45 +0200 Subject: [PATCH 15/15] Update packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js --- .../phases/3-transform/server/visitors/AssignmentExpression.js | 1 + 1 file changed, 1 insertion(+) 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 bc4e8e104ec8..3cee0a4eb8bf 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 @@ -57,6 +57,7 @@ export function AssignmentExpression(node, context) { } /** + * 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