Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
26 changes: 25 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,27 @@ export function can_inline_variable(binding) {
binding.initial?.type === 'Literal'
);
}

/**
* @param {(AST.Text | AST.ExpressionTag)[]} nodes
* @param {import('./types.js').ComponentClientTransformState} state
*/
export 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;
}
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,
create_derived,
is_inlinable_expression
} from '../utils.js';
import {
get_attribute_name,
build_attribute_value,
Expand Down Expand Up @@ -605,30 +610,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 { can_inline_variable, 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,8 +125,9 @@ 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;

Expand All @@ -139,7 +141,12 @@ function is_static_element(node) {
}

if (attribute.value !== true && !is_text_attribute(attribute)) {
return false;
// 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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning here with true could lead to bugs down the road in case a condition afterwards would return false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the subsequent checks are only for static text attributes right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh i guess except the last one which is about the node name itself...let me see what are we doing in that case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good catch !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but actually i'm stuped because the rest only checks for the name so it can be problematic there too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this check last and moved the check for node.name outside the for loop since it didn't had to be inside the loop

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh but wait it's not all actually...because i need to loop over every attribute before returning true

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok now it's fixed for real...i wonder if we should add some snapshots for this case to prevent easy regressions

Copy link
Contributor

@trueadm trueadm Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some snapshot tests, this code is very fragile without.

Array.isArray(attribute.value) ? attribute.value : [attribute.value],
state
);
}

if (attribute.name === 'autofocus' || attribute.name === 'muted') {
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);
}
Loading