chore: remove template expression inlining (#14374)

* chore: remove template expression inlining * missed some * fix * feedback * feedback * Update packages/svelte/src/compiler/phases/3-transform/client/utils.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * fix * Update .changeset/calm-mice-perform.md Co-authored-by: Rich Harris <rich.harris@vercel.com> --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
sveltejs · Nov 20, 2024 · 9d12fd1 · 9d12fd1
1 parent f616c22
commit 9d12fd1
Show file tree

Hide file tree

Showing 20 changed files with 97 additions and 261 deletions.
diff --git a/.changeset/calm-mice-perform.md b/.changeset/calm-mice-perform.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: remove template expression inlining
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js
@@ -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,
@@ -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) {
@@ -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)) {

diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js
@@ -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);
 		}
 	}
 }
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js
@@ -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
@@ -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 });
 }
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js
@@ -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';
@@ -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' &&
@@ -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);
@@ -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);
-	}
 }
diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js
@@ -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)) {

diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/RegularElement.js
@@ -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
@@ -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 &&

diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js
@@ -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') {

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js
@@ -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';
 
 /**

diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js
@@ -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
 			});
@@ -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;
 

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
@@ -1,37 +1,37 @@
-/** @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
 } from './shared/utils.js';
+import { visit_event_attribute } from './shared/events.js';
 
 /**
  * @param {AST.RegularElement} node
@@ -352,32 +352,30 @@ 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.type === 'ExpressionTag') &&
 		trimmed.every((node) => node.type === 'Text' || !node.metadata.expression.has_state) &&
-		trimmed.some((node) => node.type === 'ExpressionTag' && !node.metadata.expression.can_inline);
+		trimmed.some((node) => node.type === 'ExpressionTag');
 
 	if (use_text_content) {
-		let { value } = build_template_chunk(trimmed, context.visit, child_state);
-
 		child_state.init.push(
-			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`
@@ -387,7 +385,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
 		});
@@ -587,44 +585,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;
 }
 
 /**