From 3ce5ee74c46ea6938050fcf8b08baca883b00bf5 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Mon, 11 Nov 2024 22:11:49 +0100 Subject: [PATCH 01/42] fix: avoid marking subtree as dynamic for inlined attributes --- .changeset/spotty-sheep-fetch.md | 5 ++ .../2-analyze/visitors/ExpressionTag.js | 15 ++++-- .../phases/2-analyze/visitors/Identifier.js | 17 +++++- .../phases/3-transform/client/utils.js | 52 +++---------------- .../client/visitors/RegularElement.js | 24 +++------ .../client/visitors/shared/fragment.js | 4 +- packages/svelte/src/compiler/phases/utils.js | 39 ++++++++++++++ .../_expected/client/index.svelte.js | 2 - 8 files changed, 88 insertions(+), 70 deletions(-) create mode 100644 .changeset/spotty-sheep-fetch.md create mode 100644 packages/svelte/src/compiler/phases/utils.js diff --git a/.changeset/spotty-sheep-fetch.md b/.changeset/spotty-sheep-fetch.md new file mode 100644 index 000000000000..390aa8e79e00 --- /dev/null +++ b/.changeset/spotty-sheep-fetch.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid marking subtree as dynamic for inlined attributes diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/ExpressionTag.js index 32c8d2ca3671..f0e74e3dc947 100644 --- 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 { is_inlinable_expression } from '../../utils.js'; import { mark_subtree_dynamic } from './shared/fragment.js'; /** @@ -15,9 +16,17 @@ 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); + const attribute_parent = context.path.find((parent) => parent.type === 'Attribute'); + /** + * if the expression tag is part of an attribute we want to check if it's inlinable before marking + * the subtree as dynamic. This is because if it's inlinable it will be inlined in the template + * directly making the whole thing actually static. + */ + if (attribute_parent && !is_inlinable_expression(node, context.state.scope)) { + // 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 index 79dccd5a7cf5..0b38815a4068 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js @@ -7,6 +7,7 @@ import * as e from '../../../errors.js'; import * as w from '../../../warnings.js'; import { is_rune } from '../../../../utils.js'; import { mark_subtree_dynamic } from './shared/fragment.js'; +import { is_inlinable_expression } from '../../utils.js'; /** * @param {Identifier} node @@ -20,7 +21,21 @@ export function Identifier(node, context) { return; } - mark_subtree_dynamic(context.path); + const expression_tag_parent = context.path.find((parent) => parent.type === 'ExpressionTag'); + const attribute_parent = context.path.find((parent) => parent.type === 'Attribute'); + + /** + * if the identifier is part of an expression tag of an attribute we want to check if it's inlinable + * before marking the subtree as dynamic. This is because if it's inlinable it will be inlined in the template + * directly making the whole thing actually static. + */ + if ( + !attribute_parent || + !expression_tag_parent || + !is_inlinable_expression(expression_tag_parent, context.state.scope) + ) { + mark_subtree_dynamic(context.path); + } // If we are using arguments outside of a function, then throw an error if ( 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 c46090597709..910f173f79ae 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -1,18 +1,18 @@ /** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */ -/** @import { AST, Binding, SvelteNode } from '#compiler' */ +/** @import { Binding, SvelteNode } from '#compiler' */ /** @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_LAZY_INITIAL, + PROPS_IS_BINDABLE, PROPS_IS_IMMUTABLE, + PROPS_IS_LAZY_INITIAL, PROPS_IS_RUNES, - PROPS_IS_UPDATED, - PROPS_IS_BINDABLE + PROPS_IS_UPDATED } 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'; /** @@ -311,43 +311,3 @@ export function create_derived_block_argument(node, context) { export function create_derived(state, arg) { return b.call(state.analysis.runes ? '$.derived' : '$.derived_safe_equal', arg); } - -/** - * Whether a variable can be referenced directly from template string. - * @param {import('#compiler').Binding | undefined} binding - * @returns {boolean} - */ -export function can_inline_variable(binding) { - return ( - !!binding && - // in a ` - production test + production test + +

{a} + {b} = {a + b}

From d52bfe734e796aa52539bfd7ad6cd0b11019e8ea Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 13 Nov 2024 20:34:58 -0500 Subject: [PATCH 09/42] simplify/speedup by doing the work once, during analysis --- .../phases/2-analyze/visitors/Attribute.js | 4 ++ .../2-analyze/visitors/CallExpression.js | 1 + .../phases/2-analyze/visitors/Identifier.js | 29 ++++++--- .../2-analyze/visitors/MemberExpression.js | 1 + .../visitors/TaggedTemplateExpression.js | 1 + .../3-transform/client/visitors/Fragment.js | 2 +- .../client/visitors/RegularElement.js | 6 +- .../client/visitors/shared/fragment.js | 4 +- packages/svelte/src/compiler/phases/nodes.js | 3 +- packages/svelte/src/compiler/phases/utils.js | 62 +++---------------- packages/svelte/src/compiler/types/index.d.ts | 2 + 11 files changed, 44 insertions(+), 71 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js index 2a281a1aa3d9..efdbe292d0ee 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js @@ -24,6 +24,10 @@ export function Attribute(node, context) { } } + if (node.name.startsWith('on')) { + mark_subtree_dynamic(context.path); + } + if (node.value !== true) { for (const chunk of get_attribute_chunks(node.value)) { if (chunk.type !== 'ExpressionTag') continue; diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js index 957b27ae9bc8..2ae32e80e1ba 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/CallExpression.js @@ -178,6 +178,7 @@ 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; } } } diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js index f0f9b1e1fb94..5dbdc232545b 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/Identifier.js @@ -23,15 +23,6 @@ export function Identifier(node, context) { const attribute_parent = context.path.find((parent) => parent.type === 'Attribute'); - /** - * if the identifier is part of an expression tag of an attribute we want to check if it's inlinable - * before marking the subtree as dynamic. This is because if it's inlinable it will be inlined in the template - * directly making the whole thing actually static. - */ - if (!attribute_parent || !is_inlinable_expression(attribute_parent.value, context.state.scope)) { - mark_subtree_dynamic(context.path); - } - // If we are using arguments outside of a function, then throw an error if ( node.name === 'arguments' && @@ -101,6 +92,13 @@ export function Identifier(node, context) { if (context.state.expression) { context.state.expression.dependencies.add(binding); context.state.expression.has_state ||= binding.kind !== 'normal'; + + // if the binding is outside module scope, the expression + // cannot be inlined (TODO allow inlining in more cases, + // e.g. primitive consts) + if (!!binding.scope.parent) { + context.state.expression.can_inline = false; + } } if ( @@ -131,5 +129,18 @@ export function Identifier(node, context) { ) { w.reactive_declaration_module_script_dependency(node); } + } else if (context.state.expression) { + // no binding means global, and we can't inline e.g. `{location}` + // because it could change between component renders + context.state.expression.can_inline = false; + } + + /** + * if the identifier is part of an expression tag of an attribute we want to check if it's inlinable + * before marking the subtree as dynamic. This is because if it's inlinable it will be inlined in the template + * directly making the whole thing actually static. + */ + if (!attribute_parent || !is_inlinable_expression(attribute_parent.value)) { + 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 index 171a1106a8ce..adcc2da4226e 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/MemberExpression.js @@ -19,6 +19,7 @@ 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; } if (!is_safe_identifier(node, context.state.scope)) { diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js index eacb8a342ac2..724b9af31185 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/TaggedTemplateExpression.js @@ -10,6 +10,7 @@ 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/visitors/Fragment.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js index 4d42b841f2b6..d0b6d0996e5a 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js @@ -144,7 +144,7 @@ export function Fragment(node, context) { const use_space_template = trimmed.some((node) => node.type === 'ExpressionTag') && trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag') && - !is_inlinable_expression(trimmed, context.state.scope); + !is_inlinable_expression(trimmed); if (use_space_template) { // special case — we can use `$.text` instead of creating a unique template 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 e5d1d89f5f89..097869104e9e 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 @@ -363,7 +363,7 @@ export function RegularElement(node, context) { if (states_and_calls && states_and_calls.states === 0) { let { value } = build_template_literal(trimmed, context.visit, child_state); // if the expression is inlinable we just push it to the template - if (is_inlinable_expression(trimmed, context.state.scope)) { + if (is_inlinable_expression(trimmed)) { escape_template_quasis(value); state.template.push(value); } else { @@ -381,7 +381,7 @@ export function RegularElement(node, context) { let needs_reset = trimmed.some((node) => node.type !== 'Text') && (!trimmed.every((node) => node.type === 'Text' || node.type === 'ExpressionTag') || - !is_inlinable_expression(trimmed, context.state.scope)); + !is_inlinable_expression(trimmed)); // The same applies if it's a `