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

chore: remove template expression inlining #14374

Merged
merged 13 commits into from
Nov 20, 2024
Merged
5 changes: 5 additions & 0 deletions .changeset/calm-mice-perform.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

chore: remove template expression inlining
trueadm marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression } from 'estree' */
/** @import { AST, DelegatedEvent, SvelteNode } from '#compiler' */
/** @import { Context } from '../types' */
import { is_boolean_attribute, is_capture_event, is_delegated } from '../../../../utils.js';
import { cannot_be_set_statically, is_capture_event, is_delegated } from '../../../../utils.js';
import {
get_attribute_chunks,
get_attribute_expression,
Expand Down Expand Up @@ -30,12 +30,12 @@ export function Attribute(node, context) {
}
}

if (node.name.startsWith('on')) {
if (is_event_attribute(node)) {
mark_subtree_dynamic(context.path);
}

if (parent.type === 'RegularElement' && is_boolean_attribute(node.name.toLowerCase())) {
node.metadata.expression.can_inline = false;
if (cannot_be_set_statically(node.name)) {
mark_subtree_dynamic(context.path);
}

if (node.value !== true) {
Expand All @@ -51,7 +51,6 @@ export function Attribute(node, context) {

node.metadata.expression.has_state ||= chunk.metadata.expression.has_state;
node.metadata.expression.has_call ||= chunk.metadata.expression.has_call;
node.metadata.expression.can_inline &&= chunk.metadata.expression.can_inline;
}

if (is_event_attribute(node)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,6 @@ export function CallExpression(node, context) {
if (!is_pure(node.callee, context) || context.state.expression.dependencies.size > 0) {
context.state.expression.has_call = true;
context.state.expression.has_state = true;
context.state.expression.can_inline = false;
mark_subtree_dynamic(context.path);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
/** @import { Context } from '../types' */
import { is_tag_valid_with_parent } from '../../../../html-tree-validation.js';
import * as e from '../../../errors.js';
import { mark_subtree_dynamic } from './shared/fragment.js';

/**
* @param {AST.ExpressionTag} node
Expand All @@ -14,5 +15,9 @@ export function ExpressionTag(node, context) {
}
}

// TODO ideally we wouldn't do this here, we'd just do it on encountering
// an `Identifier` within the tag. But we currently need to handle `{42}` etc
mark_subtree_dynamic(context.path);

context.next({ ...context.state, expression: node.metadata.expression });
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/** @import { Expression, Identifier } from 'estree' */
/** @import { EachBlock } from '#compiler' */
/** @import { Context } from '../types' */
import is_reference from 'is-reference';
import { should_proxy } from '../../3-transform/client/utils.js';
Expand All @@ -19,6 +20,8 @@ export function Identifier(node, context) {
return;
}

mark_subtree_dynamic(context.path);

// If we are using arguments outside of a function, then throw an error
if (
node.name === 'arguments' &&
Expand Down Expand Up @@ -84,12 +87,6 @@ export function Identifier(node, context) {
}
}

// no binding means global, and we can't inline e.g. `<span>{location}</span>`
// because it could change between component renders. if there _is_ a
// binding and it is outside module scope, the expression cannot
// be inlined (TODO allow inlining in more cases - e.g. primitive consts)
let can_inline = !!binding && !binding.scope.parent && binding.kind === 'normal';

if (binding) {
if (context.state.expression) {
context.state.expression.dependencies.add(binding);
Expand Down Expand Up @@ -125,12 +122,4 @@ export function Identifier(node, context) {
w.reactive_declaration_module_script_dependency(node);
}
}

if (!can_inline) {
if (context.state.expression) {
context.state.expression.can_inline = false;
}

mark_subtree_dynamic(context.path);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ export function MemberExpression(node, context) {

if (context.state.expression && !is_pure(node, context)) {
context.state.expression.has_state = true;
context.state.expression.can_inline = false;

mark_subtree_dynamic(context.path);
}

if (!is_safe_identifier(node, context.state.scope)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/** @import { AST } from '#compiler' */
/** @import { Context } from '../types' */
import { cannot_be_set_statically, is_mathml, is_svg, is_void } from '../../../../utils.js';
import { is_mathml, is_svg, is_void } from '../../../../utils.js';
import {
is_tag_valid_with_ancestor,
is_tag_valid_with_parent
Expand Down Expand Up @@ -75,14 +75,6 @@ export function RegularElement(node, context) {
node.attributes.push(create_attribute('value', child.start, child.end, [child]));
}

if (
node.attributes.some(
(attribute) => attribute.type === 'Attribute' && cannot_be_set_statically(attribute.name)
)
) {
mark_subtree_dynamic(context.path);
}

const binding = context.state.scope.get(node.name);
if (
binding !== null &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export function TaggedTemplateExpression(node, context) {
if (context.state.expression && !is_pure(node.tag, context)) {
context.state.expression.has_call = true;
context.state.expression.has_state = true;
context.state.expression.can_inline = false;
}

if (node.tag.type === 'Identifier') {
Expand Down
10 changes: 5 additions & 5 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,16 @@
/** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
/** @import { Analysis } from '../../types.js' */
/** @import { Scope } from '../../scope.js' */
import * as b from '../../../utils/builders.js';
import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js';
import {
PROPS_IS_BINDABLE,
PROPS_IS_IMMUTABLE,
PROPS_IS_LAZY_INITIAL,
PROPS_IS_IMMUTABLE,
PROPS_IS_RUNES,
PROPS_IS_UPDATED
PROPS_IS_UPDATED,
PROPS_IS_BINDABLE
} from '../../../../constants.js';
import { dev } from '../../../state.js';
import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js';
import * as b from '../../../utils/builders.js';
import { get_value } from './visitors/shared/declarations.js';

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,14 @@ export function Fragment(node, context) {
const id = b.id(context.state.scope.generate('fragment'));

const use_space_template =
trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag') &&
trimmed.some((node) => node.type === 'ExpressionTag' && !node.metadata.expression.can_inline);
trimmed.some((node) => node.type === 'ExpressionTag') &&
trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag');

if (use_space_template) {
// special case — we can use `$.text` instead of creating a unique template
const id = b.id(context.state.scope.generate('text'));

process_children(trimmed, () => id, null, {
process_children(trimmed, () => id, false, {
...context,
state
});
Expand All @@ -158,12 +158,12 @@ export function Fragment(node, context) {
} else {
if (is_standalone) {
// no need to create a template, we can just use the existing block's anchor
process_children(trimmed, () => b.id('$$anchor'), null, { ...context, state });
process_children(trimmed, () => b.id('$$anchor'), false, { ...context, state });
} else {
/** @type {(is_text: boolean) => Expression} */
const expression = (is_text) => b.call('$.first_child', id, is_text && b.true);

process_children(trimmed, expression, null, { ...context, state });
process_children(trimmed, expression, false, { ...context, state });

let flags = TEMPLATE_FRAGMENT;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,38 @@
/** @import { Expression, ExpressionStatement, Identifier, Literal, MemberExpression, ObjectExpression, Statement } from 'estree' */
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, ObjectExpression, Statement } from 'estree' */
/** @import { AST } from '#compiler' */
/** @import { SourceLocation } from '#shared' */
/** @import { ComponentClientTransformState, ComponentContext } from '../types' */
/** @import { Scope } from '../../../scope' */
import { escape_html } from '../../../../../escaping.js';
import {
cannot_be_set_statically,
is_boolean_attribute,
is_dom_property,
is_load_error_element,
is_void
} from '../../../../../utils.js';
import { escape_html } from '../../../../../escaping.js';
import { dev, is_ignored, locator } from '../../../../state.js';
import { is_event_attribute, is_text_attribute } from '../../../../utils/ast.js';
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, create_derived } from '../utils.js';
import {
get_attribute_name,
build_attribute_value,
build_class_directives,
build_set_attributes,
build_style_directives,
get_attribute_name
build_set_attributes
} from './shared/element.js';
import { visit_event_attribute } from './shared/events.js';
import { process_children } from './shared/fragment.js';
import {
build_render_statement,
build_template_chunk,
build_update,
build_update_assignment
build_update_assignment,
get_states_and_calls
} from './shared/utils.js';
trueadm marked this conversation as resolved.
Show resolved Hide resolved
import { visit_event_attribute } from './shared/events.js';

/**
* @param {AST.RegularElement} node
Expand Down Expand Up @@ -352,32 +353,28 @@ export function RegularElement(node, context) {

// special case — if an element that only contains text, we don't need
// to descend into it if the text is non-reactive
const is_text = trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag');

// in the rare case that we have static text that can't be inlined
// (e.g. `<span>{location}</span>`), set `textContent` programmatically
const use_text_content =
is_text &&
trimmed.every((node) => node.type === 'Text' || !node.metadata.expression.has_state) &&
trimmed.some((node) => node.type === 'ExpressionTag' && !node.metadata.expression.can_inline);

if (use_text_content) {
let { value } = build_template_chunk(trimmed, context.visit, child_state);
const states_and_calls =
trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag') &&
trimmed.some((node) => node.type === 'ExpressionTag') &&
get_states_and_calls(trimmed);

trueadm marked this conversation as resolved.
Show resolved Hide resolved
if (states_and_calls && states_and_calls.states === 0) {
child_state.init.push(
trueadm marked this conversation as resolved.
Show resolved Hide resolved
b.stmt(b.assignment('=', b.member(context.state.node, 'textContent'), value))
b.stmt(
b.assignment(
'=',
b.member(context.state.node, 'textContent'),
build_template_chunk(trimmed, context.visit, child_state).value
)
)
);
} else {
/** @type {Expression} */
let arg = context.state.node;

// If `hydrate_node` is set inside the element, we need to reset it
// after the element has been hydrated (we don't need to reset if it's been inlined)
let needs_reset = !trimmed.every(
(node) =>
node.type === 'Text' ||
(node.type === 'ExpressionTag' && node.metadata.expression.can_inline)
);
// after the element has been hydrated
let needs_reset = trimmed.some((node) => node.type !== 'Text');

// The same applies if it's a `<template>` element, since we need to
// set the value of `hydrate_node` to `node.content`
Expand All @@ -387,7 +384,7 @@ export function RegularElement(node, context) {
arg = b.member(arg, 'content');
}

process_children(trimmed, (is_text) => b.call('$.child', arg, is_text && b.true), node, {
process_children(trimmed, (is_text) => b.call('$.child', arg, is_text && b.true), true, {
...context,
state: child_state
});
Expand Down Expand Up @@ -587,44 +584,10 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
state.update.push(update);
}
return true;
}

// we need to special case textarea value because it's not an actual attribute
const can_inline =
(attribute.name !== 'value' || element.name !== 'textarea') &&
attribute.metadata.expression.can_inline;

if (can_inline) {
/** @type {Literal | undefined} */
let literal = undefined;

if (value.type === 'Literal') {
literal = value;
} else if (value.type === 'Identifier') {
const binding = context.state.scope.get(value.name);
if (binding && binding.initial?.type === 'Literal' && !binding.reassigned) {
literal = binding.initial;
}
}

if (literal && escape_html(literal.value, true) === String(literal.value)) {
if (is_boolean_attribute(name)) {
if (literal.value) {
context.state.template.push(` ${name}`);
}
} else {
context.state.template.push(` ${name}="`, value, '"');
}
} else {
context.state.template.push(
b.call('$.attr', b.literal(name), value, is_boolean_attribute(name) && b.true)
);
}
} else {
state.init.push(update);
return false;
}

return false;
}

/**
Expand Down
Loading
Loading