Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: consider static attributes that are inlined in the template #14249

Merged
merged 8 commits into from
Nov 11, 2024
5 changes: 5 additions & 0 deletions .changeset/mighty-ads-appear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: consider static attributes that are inlined in the template
27 changes: 26 additions & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */
/** @import { Binding, SvelteNode } from '#compiler' */
/** @import { AST, Binding, SvelteNode } from '#compiler' */
/** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
/** @import { Analysis } from '../../types.js' */
/** @import { Scope } from '../../scope.js' */
Expand Down Expand Up @@ -326,3 +326,28 @@ export function can_inline_variable(binding) {
binding.initial?.type === 'Literal'
);
}

/**
* @param {(AST.Text | AST.ExpressionTag) | (AST.Text | AST.ExpressionTag)[]} node_or_nodes
* @param {import('./types.js').ComponentClientTransformState} 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') {
if (value.expression.type === 'Identifier') {
const binding = state.scope
.owner(value.expression.name)
?.declarations.get(value.expression.name);
if (!can_inline_variable(binding)) {
return false;
}
} else {
return false;
}
has_expression_tag = true;
}
}
return has_expression_tag;
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@ import {
import * as b from '../../../../utils/builders.js';
import { is_custom_element_node } from '../../../nodes.js';
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
import { build_getter, can_inline_variable, create_derived } from '../utils.js';
import {
build_getter,
can_inline_variable,
paoloricciuti marked this conversation as resolved.
Show resolved Hide resolved
create_derived,
is_inlinable_expression
} from '../utils.js';
import {
get_attribute_name,
build_attribute_value,
Expand Down Expand Up @@ -584,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));
Expand All @@ -605,30 +607,6 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
}
}

/**
* @param {(AST.Text | AST.ExpressionTag)[]} nodes
* @param {import('../types.js').ComponentClientTransformState} state
*/
function is_inlinable_expression(nodes, state) {
let has_expression_tag = false;
for (let value of nodes) {
if (value.type === 'ExpressionTag') {
if (value.expression.type === 'Identifier') {
const binding = state.scope
.owner(value.expression.name)
?.declarations.get(value.expression.name);
if (!can_inline_variable(binding)) {
return false;
}
} else {
return false;
}
has_expression_tag = true;
}
}
return has_expression_tag;
}

/**
* Like `build_element_attribute_update_assignment` but without any special attribute treatment.
* @param {Identifier} node_id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 { is_inlinable_expression } from '../../utils.js';
import { build_template_literal, build_update } from './utils.js';

/**
Expand Down Expand Up @@ -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)) {
skipped += 1;
} else if (node.type === 'EachBlock' && nodes.length === 1 && is_element) {
node.metadata.is_controlled = true;
Expand All @@ -124,10 +125,12 @@ export function process_children(nodes, initial, is_element, { visit, state }) {

/**
* @param {SvelteNode} node
* @param {ComponentContext["state"]} state
*/
function is_static_element(node) {
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') {
Expand All @@ -138,10 +141,6 @@ function is_static_element(node) {
return false;
}

if (attribute.value !== true && !is_text_attribute(attribute)) {
return false;
}

if (attribute.name === 'autofocus' || attribute.name === 'muted') {
return false;
}
Expand All @@ -155,8 +154,14 @@ function is_static_element(node) {
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 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;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,8 @@ var root = $.template(`<picture><source srcset="${__DECLARED_ASSET_0__}" type="i

export default function Inline_module_vars($$anchor) {
var picture = root();
var source = $.child(picture);
var source_1 = $.sibling(source, 2);
var source_2 = $.sibling(source_1, 2);
var img = $.sibling(source_2, 2);

$.next(6);
$.reset(picture);
$.append($$anchor, picture);
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import "svelte/internal/disclose-version";
import * as $ from "svelte/internal/client";

var root = $.template(`<header><nav><a href="/">Home</a> <a href="/away">Away</a></nav></header> <main><h1> </h1> <div class="static"><p>we don't need to traverse these nodes</p></div> <p>or</p> <p>these</p> <p>ones</p> <!> <p>these</p> <p>trailing</p> <p>nodes</p> <p>can</p> <p>be</p> <p>completely</p> <p>ignored</p></main> <cant-skip><custom-elements></custom-elements></cant-skip>`, 3);
var root = $.template(`<header><nav><a href="/">Home</a> <a href="/away">Away</a></nav></header> <main><h1> </h1> <div class="static"><p>we don't need to traverse these nodes</p></div> <p>or</p> <p>these</p> <p>ones</p> <!> <p>these</p> <p>trailing</p> <p>nodes</p> <p>can</p> <p>be</p> <p>completely</p> <p>ignored</p></main> <cant-skip><custom-elements></custom-elements></cant-skip> <div><input></div> <div><source></div> <select><option>a</option></select> <img src="..." alt="" loading="lazy">`, 3);

export default function Skip_static_subtree($$anchor, $$props) {
var fragment = root();
Expand All @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import * as $ from "svelte/internal/server";
export default function Skip_static_subtree($$payload, $$props) {
let { title, content } = $$props;

$$payload.out += `<header><nav><a href="/">Home</a> <a href="/away">Away</a></nav></header> <main><h1>${$.escape(title)}</h1> <div class="static"><p>we don't need to traverse these nodes</p></div> <p>or</p> <p>these</p> <p>ones</p> ${$.html(content)} <p>these</p> <p>trailing</p> <p>nodes</p> <p>can</p> <p>be</p> <p>completely</p> <p>ignored</p></main> <cant-skip><custom-elements with="attributes"></custom-elements></cant-skip>`;
$$payload.out += `<header><nav><a href="/">Home</a> <a href="/away">Away</a></nav></header> <main><h1>${$.escape(title)}</h1> <div class="static"><p>we don't need to traverse these nodes</p></div> <p>or</p> <p>these</p> <p>ones</p> ${$.html(content)} <p>these</p> <p>trailing</p> <p>nodes</p> <p>can</p> <p>be</p> <p>completely</p> <p>ignored</p></main> <cant-skip><custom-elements with="attributes"></custom-elements></cant-skip> <div><input autofocus></div> <div><source muted></div> <select><option value="a">a</option></select> <img src="..." alt="" loading="lazy">`;
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,18 @@
<cant-skip>
<custom-elements with="attributes"></custom-elements>
</cant-skip>

<div>
<!-- svelte-ignore a11y_autofocus -->
<input autofocus />
</div>

<div>
<source muted />
</div>

<select>
<option value="a">a</option>
</select>

<img src="..." alt="" loading="lazy" />
Loading