From bc2d30c5582f2b1471b8426aa403d1af4adec121 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 20 Sep 2024 13:59:20 -0400 Subject: [PATCH] chore: refactor `set_attributes` code generation (#13353) * chore: refactor `set_attributes` code generation * simplify * simplify --- .../client/visitors/RegularElement.js | 160 +++++------------- .../client/visitors/SvelteElement.js | 130 +++----------- .../client/visitors/shared/element.js | 88 +++++++++- .../client/visitors/shared/utils.js | 1 + .../server/visitors/shared/element.js | 9 +- .../client/dom/elements/attributes.js | 8 +- 6 files changed, 163 insertions(+), 233 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js index fd8aeb6e4c..911991b90a 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js @@ -24,7 +24,8 @@ import { get_attribute_name, build_attribute_value, build_class_directives, - build_style_directives + build_style_directives, + build_set_attributes } from './shared/element.js'; import { process_children } from './shared/fragment.js'; import { @@ -95,7 +96,7 @@ export function RegularElement(node, context) { /** @type {Map} */ const bindings = new Map(); - let has_spread = false; + let has_spread = node.metadata.has_spread; let has_use = false; for (const attribute of node.attributes) { @@ -105,6 +106,16 @@ export function RegularElement(node, context) { break; case 'Attribute': + // `is` attributes need to be part of the template, otherwise they break + if (attribute.name === 'is' && context.state.metadata.namespace === 'html') { + const { value } = build_attribute_value(attribute.value, context); + + if (value.type === 'Literal' && typeof value.value === 'string') { + context.state.template.push(` is="${escape_html(value.value, true)}"`); + continue; + } + } + attributes.push(attribute); lookup.set(attribute.name, attribute); break; @@ -129,7 +140,6 @@ export function RegularElement(node, context) { case 'SpreadAttribute': attributes.push(attribute); - has_spread = true; break; case 'StyleDirective': @@ -194,17 +204,40 @@ export function RegularElement(node, context) { const node_id = context.state.node; // Then do attributes - let is_attributes_reactive = false; - if (node.metadata.has_spread) { - build_element_spread_attributes( + let is_attributes_reactive = has_spread; + + if (has_spread) { + const attributes_id = b.id(context.state.scope.generate('attributes')); + + build_set_attributes( attributes, context, node, node_id, - // If value binding exists, that one takes care of calling $.init_select - node.name === 'select' && !bindings.has('value') + attributes_id, + (node.metadata.svg || node.metadata.mathml || is_custom_element_node(node)) && b.true, + node.name.includes('-') && b.true ); - is_attributes_reactive = true; + + // If value binding exists, that one takes care of calling $.init_select + if (node.name === 'select' && !bindings.has('value')) { + context.state.init.push( + b.stmt(b.call('$.init_select', node_id, b.thunk(b.member(attributes_id, 'value')))) + ); + + context.state.update.push( + b.if( + b.binary('in', b.literal('value'), attributes_id), + b.block([ + // This ensures a one-way street to the DOM in case it's . We need it in addition to $.init_select + // because the select value is not reflected as an attribute, so the + // mutation observer wouldn't notice. + b.stmt(b.call('$.select_option', node_id, b.member(attributes_id, 'value'))) + ]) + ) + ); + } } else { /** If true, needs `__value` for inputs */ const needs_special_value_handling = @@ -229,7 +262,7 @@ export function RegularElement(node, context) { attribute.name !== 'autofocus' && (attribute.value === true || is_text_attribute(attribute)) ) { - const name = get_attribute_name(node, attribute, context); + const name = get_attribute_name(node, attribute); const value = is_text_attribute(attribute) ? attribute.value[0].data : true; if (name !== 'class' || value) { @@ -258,7 +291,7 @@ export function RegularElement(node, context) { node_id, context, is_attributes_reactive, - lookup.has('style') || node.metadata.has_spread + lookup.has('style') || has_spread ); // Apply the src and loading attributes for elements after the element is appended to the document @@ -448,109 +481,6 @@ function setup_select_synchronization(value_binding, context) { ); } -/** - * @param {Array} attributes - * @param {ComponentContext} context - * @param {AST.RegularElement} element - * @param {Identifier} element_id - * @param {boolean} needs_select_handling - */ -function build_element_spread_attributes( - attributes, - context, - element, - element_id, - needs_select_handling -) { - let needs_isolation = false; - - /** @type {ObjectExpression['properties']} */ - const values = []; - - for (const attribute of attributes) { - if (attribute.type === 'Attribute') { - const name = get_attribute_name(element, attribute, context); - // TODO: handle has_call - const { value } = build_attribute_value(attribute.value, context); - - if ( - name === 'is' && - value.type === 'Literal' && - context.state.metadata.namespace === 'html' - ) { - context.state.template.push(` is="${escape_html(value.value, true)}"`); - continue; - } - - if ( - is_event_attribute(attribute) && - (get_attribute_expression(attribute).type === 'ArrowFunctionExpression' || - get_attribute_expression(attribute).type === 'FunctionExpression') - ) { - // Give the event handler a stable ID so it isn't removed and readded on every update - const id = context.state.scope.generate('event_handler'); - context.state.init.push(b.var(id, value)); - values.push(b.init(attribute.name, b.id(id))); - } else { - values.push(b.init(name, value)); - } - } else { - values.push(b.spread(/** @type {Expression} */ (context.visit(attribute)))); - } - - needs_isolation ||= - attribute.type === 'SpreadAttribute' && attribute.metadata.expression.has_call; - } - - const preserve_attribute_case = - element.metadata.svg || element.metadata.mathml || is_custom_element_node(element); - const id = b.id(context.state.scope.generate('attributes')); - - const update = b.stmt( - b.assignment( - '=', - id, - b.call( - '$.set_attributes', - element_id, - id, - b.object(values), - context.state.analysis.css.hash !== '' && b.literal(context.state.analysis.css.hash), - preserve_attribute_case && b.true, - is_ignored(element, 'hydration_attribute_changed') && b.true, - element.name.includes('-') && b.true - ) - ) - ); - - context.state.init.push(b.let(id)); - - // objects could contain reactive getters -> play it safe and always assume spread attributes are reactive - if (needs_isolation) { - context.state.init.push(build_update(update)); - } else { - context.state.update.push(update); - } - - if (needs_select_handling) { - context.state.init.push( - b.stmt(b.call('$.init_select', element_id, b.thunk(b.member(id, 'value')))) - ); - context.state.update.push( - b.if( - b.binary('in', b.literal('value'), id), - b.block([ - // This ensures a one-way street to the DOM in case it's . We need it in addition to $.init_select - // because the select value is not reflected as an attribute, so the - // mutation observer wouldn't notice. - b.stmt(b.call('$.select_option', element_id, b.member(id, 'value'))) - ]) - ) - ); - } -} - /** * Serializes an assignment to an element property by adding relevant statements to either only * the init or the the init and update arrays, depending on whether or not the value is dynamic. @@ -581,7 +511,7 @@ function build_element_spread_attributes( */ function build_element_attribute_update_assignment(element, node_id, attribute, context) { const state = context.state; - const name = get_attribute_name(element, attribute, context); + const name = get_attribute_name(element, attribute); const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg'; const is_mathml = context.state.metadata.namespace === 'mathml'; let { has_call, value } = build_attribute_value(attribute.value, context); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js index 01bd970035..85a2b04d91 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js @@ -12,6 +12,7 @@ import { determine_namespace_for_children } from '../../utils.js'; import { build_attribute_value, build_class_directives, + build_set_attributes, build_style_directives } from './shared/element.js'; import { build_render_statement, build_update } from './shared/utils.js'; @@ -81,10 +82,29 @@ export function SvelteElement(node, context) { context.state.init.push(...lets); // create computeds in the outer context; the dynamic element is the single child of this slot // Then do attributes - // Always use spread because we don't know whether the element is a custom element or not, - // therefore we need to do the "how to set an attribute" logic at runtime. - const is_attributes_reactive = - build_dynamic_element_attributes(node, attributes, inner_context, element_id) !== null; + let is_attributes_reactive = false; + + if (attributes.length === 0) { + if (context.state.analysis.css.hash) { + inner_context.state.init.push( + b.stmt(b.call('$.set_class', element_id, b.literal(context.state.analysis.css.hash))) + ); + } + } else { + const attributes_id = b.id(context.state.scope.generate('attributes')); + + // Always use spread because we don't know whether the element is a custom element or not, + // therefore we need to do the "how to set an attribute" logic at runtime. + is_attributes_reactive = build_set_attributes( + attributes, + inner_context, + node, + element_id, + attributes_id, + b.binary('!==', b.member(element_id, 'namespaceURI'), b.id('$.NAMESPACE_SVG')), + b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-')) + ); + } // class/style directives must be applied last since they could override class/style attributes build_class_directives(class_directives, element_id, inner_context, is_attributes_reactive); @@ -133,105 +153,3 @@ export function SvelteElement(node, context) { ) ); } - -/** - * Serializes dynamic element attribute assignments. - * Returns the `true` if spread is deemed reactive. - * @param {AST.SvelteElement} element - * @param {Array} attributes - * @param {ComponentContext} context - * @param {Identifier} element_id - * @returns {boolean} - */ -function build_dynamic_element_attributes(element, attributes, context, element_id) { - if (attributes.length === 0) { - if (context.state.analysis.css.hash) { - context.state.init.push( - b.stmt(b.call('$.set_class', element_id, b.literal(context.state.analysis.css.hash))) - ); - } - return false; - } - - // TODO why are we always treating this as a spread? needs docs, if that's not an error - - let needs_isolation = false; - let is_reactive = false; - - /** @type {ObjectExpression['properties']} */ - const values = []; - - for (const attribute of attributes) { - if (attribute.type === 'Attribute') { - const { value } = build_attribute_value(attribute.value, context); - - if ( - is_event_attribute(attribute) && - (get_attribute_expression(attribute).type === 'ArrowFunctionExpression' || - get_attribute_expression(attribute).type === 'FunctionExpression') - ) { - // Give the event handler a stable ID so it isn't removed and readded on every update - const id = context.state.scope.generate('event_handler'); - context.state.init.push(b.var(id, value)); - values.push(b.init(attribute.name, b.id(id))); - } else { - values.push(b.init(attribute.name, value)); - } - } else { - values.push(b.spread(/** @type {Expression} */ (context.visit(attribute)))); - } - - is_reactive ||= - attribute.metadata.expression.has_state || - // objects could contain reactive getters -> play it safe and always assume spread attributes are reactive - attribute.type === 'SpreadAttribute'; - needs_isolation ||= - attribute.type === 'SpreadAttribute' && attribute.metadata.expression.has_call; - } - - if (needs_isolation || is_reactive) { - const id = context.state.scope.generate('attributes'); - context.state.init.push(b.let(id)); - - const update = b.stmt( - b.assignment( - '=', - b.id(id), - b.call( - '$.set_attributes', - element_id, - b.id(id), - b.object(values), - context.state.analysis.css.hash !== '' && b.literal(context.state.analysis.css.hash), - b.binary('!==', b.member(element_id, 'namespaceURI'), b.id('$.NAMESPACE_SVG')), - is_ignored(element, 'hydration_attribute_changed') && b.true, - b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-')) - ) - ) - ); - - if (needs_isolation) { - context.state.init.push(build_update(update)); - return false; - } - - context.state.update.push(update); - return true; - } - - context.state.init.push( - b.stmt( - b.call( - '$.set_attributes', - element_id, - b.literal(null), - b.object(values), - context.state.analysis.css.hash !== '' && b.literal(context.state.analysis.css.hash), - b.binary('!==', b.member(element_id, 'namespaceURI'), b.id('$.NAMESPACE_SVG')), - is_ignored(element, 'hydration_attribute_changed') && b.true, - b.call(b.member(b.member(element_id, 'nodeName'), 'includes'), b.literal('-')) - ) - ) - ); - return false; -} diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js index b46392bf11..d2bf3d53ce 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js @@ -1,11 +1,93 @@ -/** @import { Expression, Identifier } from 'estree' */ +/** @import { Expression, Identifier, ObjectExpression } from 'estree' */ /** @import { AST, Namespace } from '#compiler' */ /** @import { ComponentContext } from '../../types' */ import { normalize_attribute } from '../../../../../../utils.js'; +import { is_ignored } from '../../../../../state.js'; +import { get_attribute_expression, is_event_attribute } from '../../../../../utils/ast.js'; import * as b from '../../../../../utils/builders.js'; import { build_getter, create_derived } from '../../utils.js'; import { build_template_literal, build_update } from './utils.js'; +/** + * @param {Array} attributes + * @param {ComponentContext} context + * @param {AST.RegularElement | AST.SvelteElement} element + * @param {Identifier} element_id + * @param {Identifier} attributes_id + * @param {false | Expression} preserve_attribute_case + * @param {false | Expression} is_custom_element + */ +export function build_set_attributes( + attributes, + context, + element, + element_id, + attributes_id, + preserve_attribute_case, + is_custom_element +) { + let needs_isolation = false; + let has_state = false; + + /** @type {ObjectExpression['properties']} */ + const values = []; + + for (const attribute of attributes) { + if (attribute.type === 'Attribute') { + const { value } = build_attribute_value(attribute.value, context); + + if ( + is_event_attribute(attribute) && + (value.type === 'ArrowFunctionExpression' || value.type === 'FunctionExpression') + ) { + // Give the event handler a stable ID so it isn't removed and readded on every update + const id = context.state.scope.generate('event_handler'); + context.state.init.push(b.var(id, value)); + values.push(b.init(attribute.name, b.id(id))); + } else { + values.push(b.init(attribute.name, value)); + } + + has_state ||= attribute.metadata.expression.has_state; + } else { + values.push(b.spread(/** @type {Expression} */ (context.visit(attribute)))); + + // objects could contain reactive getters -> play it safe and always assume spread attributes are reactive + has_state = true; + + needs_isolation ||= attribute.metadata.expression.has_call; + } + } + + const call = b.call( + '$.set_attributes', + element_id, + has_state ? attributes_id : b.literal(null), + b.object(values), + context.state.analysis.css.hash !== '' && b.literal(context.state.analysis.css.hash), + preserve_attribute_case, + is_custom_element, + is_ignored(element, 'hydration_attribute_changed') && b.true + ); + + if (has_state) { + context.state.init.push(b.let(attributes_id)); + + const update = b.stmt(b.assignment('=', attributes_id, call)); + + if (needs_isolation) { + context.state.init.push(build_update(update)); + return false; + } + + context.state.update.push(update); + return true; + } + + context.state.init.push(b.stmt(call)); + return false; +} + /** * Serializes each style directive into something like `$.set_style(element, style_property, value)` * and adds it either to init or update, depending on whether or not the value or the attributes are dynamic. @@ -101,6 +183,7 @@ export function build_class_directives( /** * @param {AST.Attribute['value']} value * @param {ComponentContext} context + * @returns {{ value: Expression, has_state: boolean, has_call: boolean }} */ export function build_attribute_value(value, context) { if (value === true) { @@ -127,9 +210,8 @@ export function build_attribute_value(value, context) { /** * @param {AST.RegularElement | AST.SvelteElement} element * @param {AST.Attribute} attribute - * @param {{ state: { metadata: { namespace: Namespace }}}} context */ -export function get_attribute_name(element, attribute, context) { +export function get_attribute_name(element, attribute) { if (!element.metadata.svg && !element.metadata.mathml) { return normalize_attribute(attribute.name); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 7d2c5d78a1..21b902d5c2 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -36,6 +36,7 @@ export function get_states_and_calls(values) { * @param {Array} values * @param {(node: SvelteNode, state: any) => any} visit * @param {ComponentClientTransformState} state + * @returns {{ value: Expression, has_state: boolean, has_call: boolean }} */ export function build_template_literal(values, visit, state) { /** @type {Expression[]} */ diff --git a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js index a968d646ad..c386c4f7c0 100644 --- a/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js +++ b/packages/svelte/src/compiler/phases/3-transform/server/visitors/shared/element.js @@ -217,7 +217,7 @@ export function build_element_attributes(node, context) { } else { for (const attribute of /** @type {AST.Attribute[]} */ (attributes)) { if (attribute.value === true || is_text_attribute(attribute)) { - const name = get_attribute_name(node, attribute, context); + const name = get_attribute_name(node, attribute); const literal_value = /** @type {Literal} */ ( build_attribute_value( attribute.value, @@ -239,7 +239,7 @@ export function build_element_attributes(node, context) { continue; } - const name = get_attribute_name(node, attribute, context); + const name = get_attribute_name(node, attribute); const value = build_attribute_value( attribute.value, context, @@ -264,9 +264,8 @@ export function build_element_attributes(node, context) { /** * @param {AST.RegularElement | AST.SvelteElement} element * @param {AST.Attribute} attribute - * @param {{ state: { namespace: Namespace }}} context */ -function get_attribute_name(element, attribute, context) { +function get_attribute_name(element, attribute) { let name = attribute.name; if (!element.metadata.svg && !element.metadata.mathml) { name = name.toLowerCase(); @@ -334,7 +333,7 @@ function build_element_spread_attributes( const object = b.object( attributes.map((attribute) => { if (attribute.type === 'Attribute') { - const name = get_attribute_name(element, attribute, context); + const name = get_attribute_name(element, attribute); const value = build_attribute_value( attribute.value, context, diff --git a/packages/svelte/src/internal/client/dom/elements/attributes.js b/packages/svelte/src/internal/client/dom/elements/attributes.js index 29e872dc24..db3ac5fa4e 100644 --- a/packages/svelte/src/internal/client/dom/elements/attributes.js +++ b/packages/svelte/src/internal/client/dom/elements/attributes.js @@ -149,9 +149,9 @@ export function set_custom_element_data(node, prop, value) { * @param {Record | undefined} prev * @param {Record} next New attributes - this function mutates this object * @param {string} [css_hash] - * @param {boolean} preserve_attribute_case - * @param {boolean} [skip_warning] + * @param {boolean} [preserve_attribute_case] * @param {boolean} [is_custom_element] + * @param {boolean} [skip_warning] * @returns {Record} */ export function set_attributes( @@ -160,8 +160,8 @@ export function set_attributes( next, css_hash, preserve_attribute_case = false, - skip_warning = false, - is_custom_element = false + is_custom_element = false, + skip_warning = false ) { var current = prev || {}; var is_option_element = element.tagName === 'OPTION';