From cf8df0bacc4a952ec5efdbe7115b6b228015e308 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 24 Jul 2024 10:19:39 -0400 Subject: [PATCH] chore: remove `binding.expression` (#12530) * add state.getters as alternative to binding.expression * on second thoughts * fix * first of many * couple more * regenerate types * more * another * more * another * another * another * remove binding.expression from client-side code * tweak * last one * comment * regenerate types * add a changeset * small tidy up * simplify * simplify * simplify and fix * simplify --- .changeset/four-peas-tickle.md | 5 + .../3-transform/client/transform-client.js | 2 + .../phases/3-transform/client/utils.js | 24 ++- .../3-transform/client/visitors/template.js | 177 ++++++++++-------- .../3-transform/server/transform-server.js | 50 +++-- .../compiler/phases/3-transform/types.d.ts | 6 + packages/svelte/src/compiler/phases/scope.js | 1 - packages/svelte/src/compiler/types/index.d.ts | 5 - packages/svelte/types/index.d.ts | 5 - 9 files changed, 154 insertions(+), 121 deletions(-) create mode 100644 .changeset/four-peas-tickle.md diff --git a/.changeset/four-peas-tickle.md b/.changeset/four-peas-tickle.md new file mode 100644 index 0000000000..3ee45e80fc --- /dev/null +++ b/.changeset/four-peas-tickle.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +chore: remove internal `binding.expression` mechanism diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index 7733871ad4..dbc7cd5ca8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -65,6 +65,7 @@ export function client_component(source, analysis, options) { preserve_whitespace: options.preserveWhitespace, public_state: new Map(), private_state: new Map(), + getters: {}, in_constructor: false, // these are set inside the `Fragment` visitor, and cannot be used until then @@ -583,6 +584,7 @@ export function client_module(analysis, options) { legacy_reactive_statements: new Map(), public_state: new Map(), private_state: new Map(), + getters: {}, in_constructor: false }; 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 5cc0f5c91b..d7dbe7b057 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -84,8 +84,9 @@ export function serialize_get_binding(node, state) { return b.call(node); } - if (binding.expression) { - return typeof binding.expression === 'function' ? binding.expression(node) : binding.expression; + if (Object.hasOwn(state.getters, node.name)) { + const getter = state.getters[node.name]; + return typeof getter === 'function' ? getter(node) : getter; } if (binding.kind === 'prop' || binding.kind === 'bindable_prop') { @@ -527,17 +528,20 @@ function get_hoistable_params(node, context) { ); } + const expression = context.state.getters[reference]; + if ( // If it's a destructured derived binding, then we can extract the derived signal reference and use that. - binding.expression !== null && - typeof binding.expression !== 'function' && - binding.expression.type === 'MemberExpression' && - binding.expression.object.type === 'CallExpression' && - binding.expression.object.callee.type === 'Identifier' && - binding.expression.object.callee.name === '$.get' && - binding.expression.object.arguments[0].type === 'Identifier' + // TODO this code is bad, we need to kill it + expression != null && + typeof expression !== 'function' && + expression.type === 'MemberExpression' && + expression.object.type === 'CallExpression' && + expression.object.callee.type === 'Identifier' && + expression.object.callee.name === '$.get' && + expression.object.arguments[0].type === 'Identifier' ) { - push_unique(b.id(binding.expression.object.arguments[0].name)); + push_unique(b.id(expression.object.arguments[0].name)); } else if ( // If we are referencing a simple $$props value, then we need to reference the object property instead (binding.kind === 'prop' || binding.kind === 'bindable_prop') && diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 030996bc32..544efc521e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -51,6 +51,7 @@ import { javascript_visitors_runes } from './javascript-runes.js'; import { sanitize_template_string } from '../../../../utils/sanitize_template_string.js'; import { walk } from 'zimmerframe'; import { locator } from '../../../../state.js'; +import is_reference from 'is-reference'; /** * @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} element @@ -939,11 +940,7 @@ function serialize_inline_component(node, component_name, context, anchor = cont const prev = fn; fn = (node_id) => { - return serialize_bind_this( - /** @type {Identifier | MemberExpression} */ (bind_this), - context, - prev(node_id) - ); + return serialize_bind_this(bind_this, prev(node_id), context); }; } @@ -996,71 +993,69 @@ function serialize_inline_component(node, component_name, context, anchor = cont /** * Serializes `bind:this` for components and elements. - * @param {Identifier | MemberExpression} bind_this + * @param {Identifier | MemberExpression} expression + * @param {Expression} value * @param {import('zimmerframe').Context} context - * @param {Expression} node - * @returns */ -function serialize_bind_this(bind_this, context, node) { - let i = 0; - /** @type {Map} */ - const each_ids = new Map(); - // Transform each reference to an each block context variable into a $$value_ variable - // by temporarily changing the `expression` of the corresponding binding. - // These $$value_ variables will be filled in by the bind_this runtime function through its last argument. +function serialize_bind_this(expression, value, { state, visit }) { + /** @type {Identifier[]} */ + const ids = []; + + /** @type {Expression[]} */ + const values = []; + + /** @type {typeof state.getters} */ + const getters = {}; + + // Pass in each context variables to the get/set functions, so that we can null out old values on teardown. // Note that we only do this for each context variables, the consequence is that the value might be stale in // some scenarios where the value is a member expression with changing computed parts or using a combination of multiple // variables, but that was the same case in Svelte 4, too. Once legacy mode is gone completely, we can revisit this. - walk( - bind_this, - {}, - { - Identifier(node) { - const binding = context.state.scope.get(node.name); - if (!binding || each_ids.has(binding)) return; - - const associated_node = Array.from(context.state.scopes.entries()).find( - ([_, scope]) => scope === binding?.scope - )?.[0]; - if (associated_node?.type === 'EachBlock') { - each_ids.set(binding, [ - i, - /** @type {Expression} */ (context.visit(node)), - binding.expression - ]); - binding.expression = b.id('$$value_' + i); - i++; + walk(expression, null, { + Identifier(node, { path }) { + if (Object.hasOwn(getters, node.name)) return; + + const parent = /** @type {Expression} */ (path.at(-1)); + if (!is_reference(node, parent)) return; + + const binding = state.scope.get(node.name); + if (!binding) return; + + for (const [owner, scope] of state.scopes) { + if (owner.type === 'EachBlock' && scope === binding.scope) { + ids.push(node); + values.push(/** @type {Expression} */ (visit(node))); + getters[node.name] = node; + break; } } } - ); + }); - const bind_this_id = /** @type {Expression} */ (context.visit(bind_this)); - const ids = Array.from(each_ids.values()).map((id) => b.id('$$value_' + id[0])); - const assignment = b.assignment('=', bind_this, b.id('$$value')); - const update = serialize_set_binding(assignment, context, () => context.visit(assignment)); + const child_state = { ...state, getters: { ...state.getters, ...getters } }; - for (const [binding, [, , expression]] of each_ids) { - // reset expressions to what they were before - binding.expression = expression; - } + const get = /** @type {Expression} */ (visit(expression, child_state)); + const set = /** @type {Expression} */ ( + visit(b.assignment('=', expression, b.id('$$value')), child_state) + ); - /** @type {Expression[]} */ - const args = [node, b.arrow([b.id('$$value'), ...ids], update), b.arrow([...ids], bind_this_id)]; // If we're mutating a property, then it might already be non-existent. // If we make all the object nodes optional, then it avoids any runtime exceptions. /** @type {Expression | Super} */ - let bind_node = bind_this_id; + let node = get; - while (bind_node?.type === 'MemberExpression') { - bind_node.optional = true; - bind_node = bind_node.object; - } - if (each_ids.size) { - args.push(b.thunk(b.array(Array.from(each_ids.values()).map((id) => id[1])))); + while (node.type === 'MemberExpression') { + node.optional = true; + node = node.object; } - return b.call('$.bind_this', ...args); + return b.call( + '$.bind_this', + value, + b.arrow([b.id('$$value'), ...ids], set), + b.arrow([...ids], get), + values.length > 0 && b.thunk(b.array(values)) + ); } /** @@ -1394,7 +1389,7 @@ function process_children(nodes, expression, is_element, { visit, state }) { const text_id = get_node_id(expression(true), state, 'text'); const update = b.stmt( - b.call('$.set_text', text_id, /** @type {Expression} */ (visit(node.expression))) + b.call('$.set_text', text_id, /** @type {Expression} */ (visit(node.expression, state))) ); if (node.metadata.contains_call_expression && !within_bound_contenteditable) { @@ -1654,6 +1649,7 @@ export const template_visitors = { after_update: [], template: [], locations: [], + getters: { ...context.state.getters }, metadata: { context: { template_needs_import_node: false, @@ -1816,6 +1812,8 @@ export const template_visitors = { ) ); + state.getters[declaration.id.name] = b.call('$.get', declaration.id); + // we need to eagerly evaluate the expression in order to hit any // 'Cannot access x before initialization' errors if (state.options.dev) { @@ -1825,21 +1823,24 @@ export const template_visitors = { const identifiers = extract_identifiers(declaration.id); const tmp = b.id(state.scope.generate('computed_const')); + const getters = { ...state.getters }; + // Make all identifiers that are declared within the following computed regular // variables, as they are not signals in that context yet for (const node of identifiers) { - const binding = /** @type {import('#compiler').Binding} */ (state.scope.get(node.name)); - binding.expression = node; + getters[node.name] = node; } + const child_state = { ...state, getters }; + // TODO optimise the simple `{ x } = y` case — we can just return `y` // instead of destructuring it only to return a new object const fn = b.arrow( [], b.block([ b.const( - /** @type {Pattern} */ (visit(declaration.id)), - /** @type {Expression} */ (visit(declaration.init)) + /** @type {Pattern} */ (visit(declaration.id, child_state)), + /** @type {Expression} */ (visit(declaration.init, child_state)) ), b.return(b.object(identifiers.map((node) => b.prop('init', node, node)))) ]) @@ -1854,8 +1855,7 @@ export const template_visitors = { } for (const node of identifiers) { - const binding = /** @type {import('#compiler').Binding} */ (state.scope.get(node.name)); - binding.expression = b.member(b.call('$.get', tmp), node); + state.getters[node.name] = b.member(b.call('$.get', tmp), node); } } }, @@ -2484,6 +2484,11 @@ export const template_visitors = { indirect_dependencies.push(...transitive_dependencies); } + const child_state = { + ...context.state, + getters: { ...context.state.getters } + }; + /** * @param {Pattern} expression_for_id * @returns {import('#compiler').Binding['mutation']} @@ -2532,17 +2537,14 @@ export const template_visitors = { : b.id(node.index); const item = each_node_meta.item; const binding = /** @type {import('#compiler').Binding} */ (context.state.scope.get(item.name)); - binding.expression = (/** @type {import("estree").Identifier} */ id) => { + const getter = (/** @type {import("estree").Identifier} */ id) => { const item_with_loc = with_loc(item, id); return b.call('$.unwrap', item_with_loc); }; + child_state.getters[item.name] = getter; if (node.index) { - const index_binding = /** @type {import('#compiler').Binding} */ ( - context.state.scope.get(node.index) - ); - - index_binding.expression = (id) => { + child_state.getters[node.index] = (id) => { const index_with_loc = with_loc(index, id); return b.call('$.unwrap', index_with_loc); }; @@ -2560,19 +2562,23 @@ export const template_visitors = { ) ); } else { - const unwrapped = binding.expression(binding.node); + const unwrapped = getter(binding.node); const paths = extract_paths(node.context); for (const path of paths) { const name = /** @type {Identifier} */ (path.node).name; const binding = /** @type {import('#compiler').Binding} */ (context.state.scope.get(name)); const needs_derived = path.has_default_value; // to ensure that default value is only called once - const fn = b.thunk(/** @type {Expression} */ (context.visit(path.expression?.(unwrapped)))); + const fn = b.thunk( + /** @type {Expression} */ (context.visit(path.expression?.(unwrapped), child_state)) + ); declarations.push( b.let(path.node, needs_derived ? b.call('$.derived_safe_equal', fn) : fn) ); - binding.expression = needs_derived ? b.call('$.get', b.id(name)) : b.call(name); + + const getter = needs_derived ? b.call('$.get', b.id(name)) : b.call(name); + child_state.getters[name] = getter; binding.mutation = create_mutation( /** @type {Pattern} */ (path.update_expression(unwrapped)) ); @@ -2580,21 +2586,23 @@ export const template_visitors = { // we need to eagerly evaluate the expression in order to hit any // 'Cannot access x before initialization' errors if (context.state.options.dev) { - declarations.push(b.stmt(binding.expression)); + declarations.push(b.stmt(getter)); } } } - const block = /** @type {BlockStatement} */ (context.visit(node.body)); + const block = /** @type {BlockStatement} */ (context.visit(node.body, child_state)); const key_function = node.key ? b.arrow( [node.context.type === 'Identifier' ? node.context : b.id('$$item'), index], declarations.length > 0 ? b.block( - declarations.concat(b.return(/** @type {Expression} */ (context.visit(node.key)))) + declarations.concat( + b.return(/** @type {Expression} */ (context.visit(node.key, child_state))) + ) ) - : /** @type {Expression} */ (context.visit(node.key)) + : /** @type {Expression} */ (context.visit(node.key, child_state)) ) : b.id('$.index'); @@ -2755,6 +2763,9 @@ export const template_visitors = { /** @type {Statement[]} */ const declarations = []; + const getters = { ...context.state.getters }; + const child_state = { ...context.state, getters }; + for (let i = 0; i < node.parameters.length; i++) { const argument = node.parameters[i]; @@ -2766,10 +2777,8 @@ export const template_visitors = { left: argument, right: b.id('$.noop') }); - const binding = /** @type {import('#compiler').Binding} */ ( - context.state.scope.get(argument.name) - ); - binding.expression = b.call(argument); + + getters[argument.name] = b.call(argument); continue; } @@ -2791,19 +2800,20 @@ export const template_visitors = { declarations.push( b.let(path.node, needs_derived ? b.call('$.derived_safe_equal', fn) : fn) ); - binding.expression = needs_derived ? b.call('$.get', b.id(name)) : b.call(name); + + getters[name] = needs_derived ? b.call('$.get', b.id(name)) : b.call(name); // we need to eagerly evaluate the expression in order to hit any // 'Cannot access x before initialization' errors if (context.state.options.dev) { - declarations.push(b.stmt(binding.expression)); + declarations.push(b.stmt(getters[name])); } } } body = b.block([ ...declarations, - .../** @type {BlockStatement} */ (context.visit(node.body)).body + .../** @type {BlockStatement} */ (context.visit(node.body, child_state)).body ]); /** @type {Expression} */ @@ -2996,7 +3006,7 @@ export const template_visitors = { break; case 'this': - call_expr = serialize_bind_this(node.expression, context, state.node); + call_expr = serialize_bind_this(node.expression, state.node, context); break; case 'textContent': case 'innerHTML': @@ -3118,7 +3128,10 @@ export const template_visitors = { const bindings = state.scope.get_bindings(node); for (const binding of bindings) { - binding.expression = b.member(b.call('$.get', b.id(name)), b.id(binding.node.name)); + state.getters[binding.node.name] = b.member( + b.call('$.get', b.id(name)), + b.id(binding.node.name) + ); } return b.const( 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 0a5d05a6ed..8cb31e3555 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 @@ -221,8 +221,9 @@ function serialize_get_binding(node, state) { ); } - if (binding.expression) { - return typeof binding.expression === 'function' ? binding.expression(node) : binding.expression; + if (Object.hasOwn(state.getters, node.name)) { + const getter = state.getters[node.name]; + return typeof getter === 'function' ? getter(node) : getter; } return node; @@ -1233,8 +1234,20 @@ const template_visitors = { throw new Error('Node should have been handled elsewhere'); }, RegularElement(node, context) { + const namespace = determine_namespace_for_children(node, context.state.namespace); + + /** @type {import('./types').ComponentServerTransformState} */ + const state = { + ...context.state, + getters: { ...context.state.getters }, + namespace, + preserve_whitespace: + context.state.preserve_whitespace || + ((node.name === 'pre' || node.name === 'textarea') && namespace !== 'foreign') + }; + context.state.template.push(b.literal(`<${node.name}`)); - const body = serialize_element_attributes(node, context); + const body = serialize_element_attributes(node, { ...context, state }); context.state.template.push(b.literal('>')); if ((node.name === 'script' || node.name === 'style') && node.fragment.nodes.length === 1) { @@ -1246,17 +1259,6 @@ const template_visitors = { return; } - const namespace = determine_namespace_for_children(node, context.state.namespace); - - /** @type {import('./types').ComponentServerTransformState} */ - const state = { - ...context.state, - namespace, - preserve_whitespace: - context.state.preserve_whitespace || - ((node.name === 'pre' || node.name === 'textarea') && namespace !== 'foreign') - }; - const { hoisted, trimmed } = clean_nodes( node, node.fragment.nodes, @@ -1339,6 +1341,7 @@ const template_visitors = { const state = { ...context.state, + getteres: { ...context.state.getters }, namespace: determine_namespace_for_children(node, context.state.namespace), template: [], init: [] @@ -1513,7 +1516,7 @@ const template_visitors = { const bindings = state.scope.get_bindings(node); for (const binding of bindings) { - binding.expression = b.member(b.id(name), b.id(binding.node.name)); + state.getters[binding.node.name] = b.member(b.id(name), b.id(binding.node.name)); } return b.const( @@ -1539,15 +1542,24 @@ const template_visitors = { return visit(node.expression); }, SvelteFragment(node, context) { + const child_state = { + ...context.state, + getters: { ...context.state.getters } + }; + for (const attribute of node.attributes) { if (attribute.type === 'LetDirective') { context.state.template.push( - /** @type {import('estree').ExpressionStatement} */ (context.visit(attribute)) + /** @type {import('estree').ExpressionStatement} */ ( + context.visit(attribute, child_state) + ) ); } } - const block = /** @type {import('estree').BlockStatement} */ (context.visit(node.fragment)); + const block = /** @type {import('estree').BlockStatement} */ ( + context.visit(node.fragment, child_state) + ); context.state.template.push(block); }, @@ -1967,6 +1979,7 @@ export function server_component(analysis, options) { namespace: options.namespace, preserve_whitespace: options.preserveWhitespace, private_derived: new Map(), + getters: {}, skip_hydration_boundaries: false }; @@ -2277,7 +2290,8 @@ export function server_module(analysis, options) { // to be present for `javascript_visitors_legacy` and so is included in module // transform state as well as component transform state legacy_reactive_statements: new Map(), - private_derived: new Map() + private_derived: new Map(), + getters: {} }; const module = /** @type {import('estree').Program} */ ( diff --git a/packages/svelte/src/compiler/phases/3-transform/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/types.d.ts index a16fd73a2d..cd0acaf995 100644 --- a/packages/svelte/src/compiler/phases/3-transform/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/types.d.ts @@ -1,10 +1,16 @@ import type { Scope } from '../scope.js'; import type { SvelteNode, ValidatedModuleCompileOptions } from '#compiler'; import type { Analysis } from '../types.js'; +import type { Expression, Identifier } from 'estree'; export interface TransformState { readonly analysis: Analysis; readonly options: ValidatedModuleCompileOptions; readonly scope: Scope; readonly scopes: Map; + /** + * A map of `[name, node]` pairs, where `Identifier` nodes matching `name` + * will be replaced with `node` (e.g. `x` -> `$.get(x)`) + */ + readonly getters: Record Expression)>; } diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index f9f208d6ae..401372b6d2 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -115,7 +115,6 @@ export class Scope { declaration_kind, is_called: false, prop_alias: null, - expression: null, mutation: null, reassigned: false, metadata: null diff --git a/packages/svelte/src/compiler/types/index.d.ts b/packages/svelte/src/compiler/types/index.d.ts index 47616f9905..f3ca084d5e 100644 --- a/packages/svelte/src/compiler/types/index.d.ts +++ b/packages/svelte/src/compiler/types/index.d.ts @@ -305,11 +305,6 @@ export interface Binding { legacy_dependencies: Binding[]; /** Legacy props: the `class` in `{ export klass as class}`. $props(): The `class` in { class: klass } = $props() */ prop_alias: string | null; - /** - * If this is set, all references should use this expression instead of the identifier name. - * If a function is given, it will be called with the identifier at that location and should return the new expression. - */ - expression: Expression | ((id: Identifier) => Expression) | null; /** If this is set, all mutations should use this expression */ mutation: ((assignment: AssignmentExpression, context: Context) => Expression) | null; /** Additional metadata, varies per binding type */ diff --git a/packages/svelte/types/index.d.ts b/packages/svelte/types/index.d.ts index ba71b15d05..b36af178df 100644 --- a/packages/svelte/types/index.d.ts +++ b/packages/svelte/types/index.d.ts @@ -955,11 +955,6 @@ declare module 'svelte/compiler' { legacy_dependencies: Binding[]; /** Legacy props: the `class` in `{ export klass as class}`. $props(): The `class` in { class: klass } = $props() */ prop_alias: string | null; - /** - * If this is set, all references should use this expression instead of the identifier name. - * If a function is given, it will be called with the identifier at that location and should return the new expression. - */ - expression: Expression | ((id: Identifier) => Expression) | null; /** If this is set, all mutations should use this expression */ mutation: ((assignment: AssignmentExpression, context: Context) => Expression) | null; /** Additional metadata, varies per binding type */