From f21a6b001422b10f6e4e567e6bae224f513b00c2 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 10 Nov 2024 15:18:15 +0100 Subject: [PATCH 1/7] fix: consider static attributes that are inlined in the template --- .changeset/mighty-ads-appear.md | 5 +++++ .../client/visitors/shared/fragment.js | 18 ++++++++++++++++-- .../_expected/client/index.svelte.js | 5 +---- 3 files changed, 22 insertions(+), 6 deletions(-) create mode 100644 .changeset/mighty-ads-appear.md diff --git a/.changeset/mighty-ads-appear.md b/.changeset/mighty-ads-appear.md new file mode 100644 index 000000000000..c1b3b6a74606 --- /dev/null +++ b/.changeset/mighty-ads-appear.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: consider static attributes that are inlined in the template 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 022b8574d974..3f2705070ea9 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 @@ -3,6 +3,7 @@ /** @import { ComponentContext } from '../../types' */ import { is_event_attribute, is_text_attribute } from '../../../../../utils/ast.js'; import * as b from '../../../../../utils/builders.js'; +import { can_inline_variable } from '../../utils.js'; import { build_template_literal, build_update } from './utils.js'; /** @@ -97,7 +98,7 @@ export function process_children(nodes, initial, is_element, { visit, state }) { let child_state = state; - if (is_static_element(node)) { + if (is_static_element(node, state.scope)) { skipped += 1; } else if (node.type === 'EachBlock' && nodes.length === 1 && is_element) { node.metadata.is_controlled = true; @@ -124,8 +125,9 @@ export function process_children(nodes, initial, is_element, { visit, state }) { /** * @param {SvelteNode} node + * @param {ComponentContext["state"]["scope"]} scope */ -function is_static_element(node) { +function is_static_element(node, scope) { if (node.type !== 'RegularElement') return false; if (node.fragment.metadata.dynamic) return false; @@ -139,6 +141,18 @@ function is_static_element(node) { } if (attribute.value !== true && !is_text_attribute(attribute)) { + // in this situation we are already inlining the binding in the template + // so even if it's not a text attribute we can consider the element static + if ( + typeof attribute.value !== 'boolean' && + !Array.isArray(attribute.value) && + attribute.value.expression.type === 'Identifier' + ) { + const binding = scope.get(attribute.value.expression.name); + if (binding) { + return can_inline_variable(binding); + } + } return false; } diff --git a/packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js index 69c6901d50fb..46ed3c1913f3 100644 --- a/packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js @@ -9,11 +9,8 @@ var root = $.template(` Date: Sun, 10 Nov 2024 21:10:12 +0100 Subject: [PATCH 3/7] fix: move check for inlinable expression as last --- .../client/visitors/shared/fragment.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) 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 cabcfaa5f30f..b07f411b14e4 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 @@ -130,6 +130,7 @@ export function process_children(nodes, initial, is_element, { visit, state }) { function is_static_element(node, state) { if (node.type !== 'RegularElement') return false; if (node.fragment.metadata.dynamic) return false; + if (node.name.includes('-')) return false; // we're setting all attributes on custom elements through properties for (const attribute of node.attributes) { if (attribute.type !== 'Attribute') { @@ -140,15 +141,6 @@ function is_static_element(node, state) { return false; } - if (attribute.value !== true && !is_text_attribute(attribute)) { - // if it's not a text attribute but it's an inlinable expression - // we will inline it in the template so we can still consider it static - return is_inlinable_expression( - Array.isArray(attribute.value) ? attribute.value : [attribute.value], - state - ); - } - if (attribute.name === 'autofocus' || attribute.name === 'muted') { return false; } @@ -162,8 +154,13 @@ function is_static_element(node, state) { return false; } - if (node.name.includes('-')) { - return false; // we're setting all attributes on custom elements through properties + if (attribute.value !== true && !is_text_attribute(attribute)) { + // if it's not a text attribute but it's an inlinable expression + // we will inline it in the template so we can still consider it static + return is_inlinable_expression( + Array.isArray(attribute.value) ? attribute.value : [attribute.value], + state + ); } } From 3a459861afb365604b137182e24a6911f1844ecb Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 10 Nov 2024 21:27:24 +0100 Subject: [PATCH 4/7] fix: simplify and correct --- .../client/visitors/shared/fragment.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) 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 b07f411b14e4..b0a623bd392d 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 @@ -154,13 +154,19 @@ function is_static_element(node, state) { return false; } - if (attribute.value !== true && !is_text_attribute(attribute)) { - // if it's not a text attribute but it's an inlinable expression - // we will inline it in the template so we can still consider it static - return is_inlinable_expression( + if ( + attribute.value !== true && + !is_text_attribute(attribute) && + // if the attribute is not a text attribute but is inlinable we will directly inline it in the + // the template so if it's not inlinable we can return false but if it is inlinable (or a text) + // attribute we keep count of the inlinable attributes so that if all the attributes are inlinable + // we deem the node as static + !is_inlinable_expression( Array.isArray(attribute.value) ? attribute.value : [attribute.value], state - ); + ) + ) { + return false; } } From ad95a0914c484f25290b06acd8234d638d020359 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 10 Nov 2024 22:44:00 +0100 Subject: [PATCH 5/7] chore: accept single node in `is_inlinable_expression` --- .../svelte/src/compiler/phases/3-transform/client/utils.js | 5 +++-- .../phases/3-transform/client/visitors/RegularElement.js | 5 +---- .../phases/3-transform/client/visitors/shared/fragment.js | 7 ++----- 3 files changed, 6 insertions(+), 11 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 4f901e4918bf..c46090597709 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -328,10 +328,11 @@ export function can_inline_variable(binding) { } /** - * @param {(AST.Text | AST.ExpressionTag)[]} nodes + * @param {(AST.Text | AST.ExpressionTag) | (AST.Text | AST.ExpressionTag)[]} node_or_nodes * @param {import('./types.js').ComponentClientTransformState} state */ -export function is_inlinable_expression(nodes, state) { +export function is_inlinable_expression(node_or_nodes, state) { + let nodes = Array.isArray(node_or_nodes) ? node_or_nodes : [node_or_nodes]; let has_expression_tag = false; for (let value of nodes) { if (value.type === 'ExpressionTag') { 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 b6f9e0a0d181..4d3cebcee6fe 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 @@ -589,10 +589,7 @@ function build_element_attribute_update_assignment(element, node_id, attribute, const inlinable_expression = attribute.value === true ? false // not an expression - : is_inlinable_expression( - Array.isArray(attribute.value) ? attribute.value : [attribute.value], - context.state - ); + : is_inlinable_expression(attribute.value, context.state); if (attribute.metadata.expression.has_state) { if (has_call) { state.init.push(build_update(update)); 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 b0a623bd392d..45b4a9da417d 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 @@ -3,7 +3,7 @@ /** @import { ComponentContext } from '../../types' */ import { is_event_attribute, is_text_attribute } from '../../../../../utils/ast.js'; import * as b from '../../../../../utils/builders.js'; -import { can_inline_variable, is_inlinable_expression } from '../../utils.js'; +import { is_inlinable_expression } from '../../utils.js'; import { build_template_literal, build_update } from './utils.js'; /** @@ -161,10 +161,7 @@ function is_static_element(node, state) { // the template so if it's not inlinable we can return false but if it is inlinable (or a text) // attribute we keep count of the inlinable attributes so that if all the attributes are inlinable // we deem the node as static - !is_inlinable_expression( - Array.isArray(attribute.value) ? attribute.value : [attribute.value], - state - ) + !is_inlinable_expression(attribute.value, state) ) { return false; } From 6c7cded75d27da9d82dac5dbb434d45fe4f4e03f Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Mon, 11 Nov 2024 09:05:59 +0100 Subject: [PATCH 6/7] chore: update comment --- .../phases/3-transform/client/visitors/shared/fragment.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) 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 45b4a9da417d..3b009fb5b589 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 @@ -157,10 +157,8 @@ function is_static_element(node, state) { if ( attribute.value !== true && !is_text_attribute(attribute) && - // if the attribute is not a text attribute but is inlinable we will directly inline it in the - // the template so if it's not inlinable we can return false but if it is inlinable (or a text) - // attribute we keep count of the inlinable attributes so that if all the attributes are inlinable - // we deem the node as static + // If the attribute is not a text attribute but is inlinable we will directly inline it in the + // the template so before returning false we need to check that the attribute is not inlinable !is_inlinable_expression(attribute.value, state) ) { return false; From 88b111592c86a10258584bea6f86e0d772810f1d Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Mon, 11 Nov 2024 14:19:04 +0100 Subject: [PATCH 7/7] chore: add snapshots for non static nodes --- .../_expected/client/index.svelte.js | 24 ++++++++++++++++++- .../_expected/server/index.svelte.js | 2 +- .../samples/skip-static-subtree/index.svelte | 15 ++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/client/index.svelte.js index 2d3cc3ba5ae0..ae79efef42c2 100644 --- a/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/client/index.svelte.js @@ -1,7 +1,7 @@ import "svelte/internal/disclose-version"; import * as $ from "svelte/internal/client"; -var root = $.template(`

we don't need to traverse these nodes

or

these

ones

these

trailing

nodes

can

be

completely

ignored

`, 3); +var root = $.template(`

we don't need to traverse these nodes

or

these

ones

these

trailing

nodes

can

be

completely

ignored

`, 3); export default function Skip_static_subtree($$anchor, $$props) { var fragment = root(); @@ -22,6 +22,28 @@ export default function Skip_static_subtree($$anchor, $$props) { $.set_custom_element_data(custom_elements, "with", "attributes"); $.reset(cant_skip); + + var div = $.sibling(cant_skip, 2); + var input = $.child(div); + + $.autofocus(input, true); + $.reset(div); + + var div_1 = $.sibling(div, 2); + var source = $.child(div_1); + + source.muted = true; + $.reset(div_1); + + var select = $.sibling(div_1, 2); + var option = $.child(select); + + option.value = null == (option.__value = "a") ? "" : "a"; + $.reset(select); + + var img = $.sibling(select, 2); + $.template_effect(() => $.set_text(text, $$props.title)); + $.handle_lazy_img(img); $.append($$anchor, fragment); } \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/server/index.svelte.js b/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/server/index.svelte.js index 2bd910508d0f..d09ac657f925 100644 --- a/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/server/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/skip-static-subtree/_expected/server/index.svelte.js @@ -3,5 +3,5 @@ import * as $ from "svelte/internal/server"; export default function Skip_static_subtree($$payload, $$props) { let { title, content } = $$props; - $$payload.out += `

${$.escape(title)}

we don't need to traverse these nodes

or

these

ones

${$.html(content)}

these

trailing

nodes

can

be

completely

ignored

`; + $$payload.out += `

${$.escape(title)}

we don't need to traverse these nodes

or

these

ones

${$.html(content)}

these

trailing

nodes

can

be

completely

ignored

`; } \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/skip-static-subtree/index.svelte b/packages/svelte/tests/snapshot/samples/skip-static-subtree/index.svelte index de399169b46b..cb73eba707d6 100644 --- a/packages/svelte/tests/snapshot/samples/skip-static-subtree/index.svelte +++ b/packages/svelte/tests/snapshot/samples/skip-static-subtree/index.svelte @@ -30,3 +30,18 @@ + +
+ + +
+ +
+ +
+ + + + \ No newline at end of file