From 31659508b0b05b99283dc91798530b3bcfa9715a Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Thu, 1 Aug 2024 23:01:58 -0400 Subject: [PATCH] fix: only create `document.title` effect if value is dynamic (#12698) * fix: dont create an effect for static title * improve build_template_literal * tidy up * changeset * simplify * simplify * tweak --- .changeset/strange-pears-perform.md | 5 +++ .../client/visitors/BindDirective.js | 3 +- .../client/visitors/RegularElement.js | 23 ++++++------ .../client/visitors/SlotElement.js | 3 +- .../client/visitors/SvelteElement.js | 4 +- .../client/visitors/TitleElement.js | 35 +++++------------- .../client/visitors/shared/component.js | 4 +- .../client/visitors/shared/element.js | 16 ++++---- .../client/visitors/shared/fragment.js | 9 +---- .../client/visitors/shared/utils.js | 37 +++++++++++-------- 10 files changed, 65 insertions(+), 74 deletions(-) create mode 100644 .changeset/strange-pears-perform.md diff --git a/.changeset/strange-pears-perform.md b/.changeset/strange-pears-perform.md new file mode 100644 index 0000000000..93a0c6a9f6 --- /dev/null +++ b/.changeset/strange-pears-perform.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: only create `document.title` effect if value is dynamic diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js index c7aa8fddf2..611e5e570d 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js @@ -209,10 +209,11 @@ export function BindDirective(node, context) { ) )?.value ); + if (value !== undefined) { group_getter = b.thunk( b.block([ - b.stmt(build_attribute_value(value, context)[1]), + b.stmt(build_attribute_value(value, context).value), b.return(/** @type {Expression} */ (context.visit(expression))) ]) ); 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 0a107943aa..b14c9bea9b 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 @@ -229,17 +229,16 @@ export function RegularElement(node, context) { (attribute.value === true || is_text_attribute(attribute)) ) { const name = get_attribute_name(node, attribute, context); - const literal_value = /** @type {Literal} */ ( - build_attribute_value(attribute.value, context)[1] - ).value; - if (name !== 'class' || literal_value) { + const value = is_text_attribute(attribute) ? attribute.value[0].data : true; + + if (name !== 'class' || value) { // TODO namespace=foreign probably doesn't want to do template stuff at all and instead use programmatic methods // to create the elements it needs. context.state.template.push( ` ${attribute.name}${ - is_boolean_attribute(name) && literal_value === true + is_boolean_attribute(name) && value === true ? '' - : `="${literal_value === true ? '' : escape_html(literal_value, true)}"` + : `="${value === true ? '' : escape_html(value, true)}"` }` ); continue; @@ -440,7 +439,7 @@ function build_element_spread_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); + const { value } = build_attribute_value(attribute.value, context); if ( name === 'is' && @@ -554,7 +553,7 @@ function build_element_attribute_update_assignment(element, node_id, attribute, const name = get_attribute_name(element, attribute, context); 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); + let { has_call, value } = build_attribute_value(attribute.value, context); // The foreign namespace doesn't have any special handling, everything goes through the attr function if (context.state.metadata.namespace === 'foreign') { @@ -636,7 +635,7 @@ function build_element_attribute_update_assignment(element, node_id, attribute, function build_custom_element_attribute_update_assignment(node_id, attribute, context) { const state = context.state; const name = attribute.name; // don't lowercase, as we set the element's property, which might be case sensitive - let [has_call, value] = build_attribute_value(attribute.value, context); + let { has_call, value } = build_attribute_value(attribute.value, context); const update = b.stmt(b.call('$.set_custom_element_data', node_id, b.literal(name), value)); @@ -665,7 +664,7 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co */ function build_element_special_value_attribute(element, node_id, attribute, context) { const state = context.state; - const [, value] = build_attribute_value(attribute.value, context); + const { value } = build_attribute_value(attribute.value, context); const inner_assignment = b.assignment( '=', @@ -676,7 +675,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont value ) ); - const is_reactive = attribute.metadata.expression.has_state; + const is_select_with_value = // attribute.metadata.dynamic would give false negatives because even if the value does not change, // the inner options could still change, so we need to always treat it as reactive @@ -699,7 +698,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont state.init.push(b.stmt(b.call('$.init_select', node_id, b.thunk(value)))); } - if (is_reactive) { + if (attribute.metadata.expression.has_state) { const id = state.scope.generate(`${node_id.name}_value`); build_update_assignment( state, diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js index a2ba5efff3..80e6a3635d 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js @@ -30,7 +30,8 @@ export function SlotElement(node, context) { if (attribute.type === 'SpreadAttribute') { spreads.push(b.thunk(/** @type {Expression} */ (context.visit(attribute)))); } else if (attribute.type === 'Attribute') { - const [, value] = build_attribute_value(attribute.value, context); + const { value } = build_attribute_value(attribute.value, context); + if (attribute.name === 'name') { name = value; is_default = false; 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 c1391d9a25..23f6ea5b6b 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 @@ -126,7 +126,7 @@ export function SvelteElement(node, context) { get_tag, node.metadata.svg || node.metadata.mathml ? b.true : b.false, inner.length > 0 && b.arrow([element_id, b.id('$$anchor')], b.block(inner)), - dynamic_namespace && b.thunk(build_attribute_value(dynamic_namespace, context)[1]), + dynamic_namespace && b.thunk(build_attribute_value(dynamic_namespace, context).value), location && b.array([b.literal(location.line), b.literal(location.column)]) ) ) @@ -161,7 +161,7 @@ function build_dynamic_element_attributes(attributes, context, element_id) { for (const attribute of attributes) { if (attribute.type === 'Attribute') { - const [, value] = build_attribute_value(attribute.value, context); + const { value } = build_attribute_value(attribute.value, context); if ( is_event_attribute(attribute) && diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/TitleElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/TitleElement.js index 93ff3cc664..b10eb34ad9 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/TitleElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/TitleElement.js @@ -8,32 +8,17 @@ import { build_template_literal } from './shared/utils.js'; * @param {ComponentContext} context */ export function TitleElement(node, context) { - // TODO throw validation error when attributes present / when children something else than text/expression tags - // TODO only create update when expression is dynamic + const { has_state, value } = build_template_literal( + /** @type {any} */ (node.fragment.nodes), + context.visit, + context.state + ); - if (node.fragment.nodes.length === 1 && node.fragment.nodes[0].type === 'Text') { - context.state.init.push( - b.stmt( - b.assignment( - '=', - b.member(b.id('$.document'), b.id('title')), - b.literal(/** @type {Text} */ (node.fragment.nodes[0]).data) - ) - ) - ); + const statement = b.stmt(b.assignment('=', b.member(b.id('$.document'), b.id('title')), value)); + + if (has_state) { + context.state.update.push(statement); } else { - context.state.update.push( - b.stmt( - b.assignment( - '=', - b.member(b.id('$.document'), b.id('title')), - build_template_literal( - /** @type {any} */ (node.fragment.nodes), - context.visit, - context.state - )[1] - ) - ) - ); + context.state.init.push(statement); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index eaeee5fdbc..d498ef10ea 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -93,7 +93,7 @@ export function build_component(node, component_name, context, anchor = context. } else if (attribute.type === 'Attribute') { if (attribute.name.startsWith('--')) { custom_css_props.push( - b.init(attribute.name, build_attribute_value(attribute.value, context)[1]) + b.init(attribute.name, build_attribute_value(attribute.value, context).value) ); continue; } @@ -106,7 +106,7 @@ export function build_component(node, component_name, context, anchor = context. has_children_prop = true; } - const [, value] = build_attribute_value(attribute.value, context); + const { value } = build_attribute_value(attribute.value, context); if (attribute.metadata.expression.has_state) { let arg = value; 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 e63593ca0e..06563cd033 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 @@ -33,7 +33,7 @@ export function build_style_directives( let value = directive.value === true ? build_getter({ name: directive.name, type: 'Identifier' }, context.state) - : build_attribute_value(directive.value, context)[1]; + : build_attribute_value(directive.value, context).value; const update = b.stmt( b.call( @@ -92,24 +92,24 @@ export function build_class_directives( /** * @param {Attribute['value']} value * @param {ComponentContext} context - * @returns {[has_call: boolean, Expression]} */ export function build_attribute_value(value, context) { if (value === true) { - return [false, b.literal(true)]; + return { has_state: false, has_call: false, value: b.literal(true) }; } if (!Array.isArray(value) || value.length === 1) { const chunk = Array.isArray(value) ? value[0] : value; if (chunk.type === 'Text') { - return [false, b.literal(chunk.data)]; + return { has_state: false, has_call: false, value: b.literal(chunk.data) }; } - return [ - chunk.metadata.expression.has_call, - /** @type {Expression} */ (context.visit(chunk.expression)) - ]; + return { + has_state: chunk.metadata.expression.has_call, + has_call: chunk.metadata.expression.has_call, + value: /** @type {Expression} */ (context.visit(chunk.expression)) + }; } return build_template_literal(value, context.visit, context.state); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js index d03efc4fc3..58ecd392fc 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js @@ -66,18 +66,13 @@ export function process_children(nodes, expression, is_element, { visit, state } state.template.push(' '); - const [has_call, value] = build_template_literal(sequence, visit, state); + const { has_state, has_call, value } = build_template_literal(sequence, visit, state); const update = b.stmt(b.call('$.set_text', text_id, value)); if (has_call && !within_bound_contenteditable) { state.init.push(build_update(update)); - } else if ( - sequence.some( - (node) => node.type === 'ExpressionTag' && node.metadata.expression.has_state - ) && - !within_bound_contenteditable - ) { + } else if (has_state && !within_bound_contenteditable) { state.update.push(update); } else { state.init.push(b.stmt(b.assignment('=', b.member(text_id, b.id('nodeValue')), value))); 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 f13830d715..b6b11c6909 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 @@ -14,26 +14,30 @@ import { locator } from '../../../../../state.js'; * @param {Array} values * @param {(node: SvelteNode, state: any) => any} visit * @param {ComponentClientTransformState} state - * @returns {[boolean, TemplateLiteral]} */ export function build_template_literal(values, visit, state) { - /** @type {TemplateElement[]} */ - const quasis = []; - /** @type {Expression[]} */ const expressions = []; + + let quasi = b.quasi(''); + const quasis = [quasi]; + let has_call = false; + let has_state = false; let contains_multiple_call_expression = false; - quasis.push(b.quasi('')); for (let i = 0; i < values.length; i++) { const node = values[i]; - if (node.type === 'ExpressionTag' && node.metadata.expression.has_call) { - if (has_call) { - contains_multiple_call_expression = true; + if (node.type === 'ExpressionTag') { + if (node.metadata.expression.has_call) { + if (has_call) { + contains_multiple_call_expression = true; + } + has_call = true; } - has_call = true; + + has_state ||= node.metadata.expression.has_state; } } @@ -41,12 +45,10 @@ export function build_template_literal(values, visit, state) { const node = values[i]; if (node.type === 'Text') { - const last = /** @type {TemplateElement} */ (quasis.at(-1)); - last.value.raw += sanitize_template_string(node.data); + quasi.value.raw += sanitize_template_string(node.data); } else if (node.type === 'ExpressionTag' && node.expression.type === 'Literal') { - const last = /** @type {TemplateElement} */ (quasis.at(-1)); if (node.expression.value != null) { - last.value.raw += sanitize_template_string(node.expression.value + ''); + quasi.value.raw += sanitize_template_string(node.expression.value + ''); } } else { if (contains_multiple_call_expression) { @@ -65,12 +67,15 @@ export function build_template_literal(values, visit, state) { } else { expressions.push(b.logical('??', visit(node.expression, state), b.literal(''))); } - quasis.push(b.quasi('', i + 1 === values.length)); + + quasi = b.quasi('', i + 1 === values.length); + quasis.push(quasi); } } - // TODO instead of this tuple, return a `{ dynamic, complex, value }` object. will DRY stuff out - return [has_call, b.template(quasis, expressions)]; + const value = b.template(quasis, expressions); + + return { value, has_state, has_call }; } /**