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.

Sign up for GitHub

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

perf: inline module variables into template #13075

Merged
merged 8 commits into from
Sep 13, 2024
Merged

perf: inline module variables into template #13075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cd8b708
perf: inline module variables into template
benmccann Aug 29, 2024
fe64b20
restrict to literals
benmccann Sep 3, 2024
0a82796
merge main
benmccann Sep 5, 2024
bab6e9d
add comment
benmccann Sep 6, 2024
317282d
simplify
benmccann Sep 6, 2024
1bfd839
Merge branch 'main' into inline-vars
benmccann Sep 11, 2024
77cff50
replace util methods with methods directly on template
benmccann Sep 11, 2024
346a8c5
snake case
benmccann Sep 11, 2024
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/eighty-dragons-search.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

perf: inline module variables into template
5 changes: 4 additions & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ export interface ComponentClientTransformState extends ClientTransformState {
/** Stuff that happens after the render effect (control blocks, dynamic elements, bindings, actions, etc) */
readonly after_update: Statement[];
/** The HTML template string */
readonly template: string[];
readonly template: {
push_quasi: (q: string) => void;
push_expression: (e: Expression) => void;
};
readonly locations: SourceLocation[];
readonly metadata: {
namespace: Namespace;
Expand Down
15 changes: 15 additions & 0 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,3 +311,18 @@ 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 `<script module>` block
!binding.scope.parent &&
benmccann marked this conversation as resolved.
Show resolved Hide resolved
// to prevent the need for escaping
binding.initial?.type === 'Literal'
);
}
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { create_derived_block_argument } from '../utils.js';
* @param {ComponentContext} context
*/
export function AwaitBlock(node, context) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');

// Visit {#await <expression>} first to ensure that scopes are in the correct order
const expression = b.thunk(/** @type {Expression} */ (context.visit(node.expression)));
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/Comment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
*/
export function Comment(node, context) {
// We'll only get here if comments are not filtered out, which they are unless preserveComments is true
context.state.template.push(`<!--${node.data}-->`);
context.state.template.push_quasi(`<!--${node.data}-->`);
}
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export function EachBlock(node, context) {
);

if (!each_node_meta.is_controlled) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');
}

if (each_node_meta.array_name !== null) {
Expand Down
36 changes: 32 additions & 4 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/Fragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,34 @@ export function Fragment(node, context) {
/** @type {Statement | undefined} */
let close = undefined;

/** @type {string[]} */
const quasi = [];
/** @type {Expression[]} */
const expressions = [];

/** @type {ComponentClientTransformState} */
const state = {
...context.state,
before_init: [],
init: [],
update: [],
after_update: [],
template: [],
template: {
push_quasi: (/** @type {string} */ quasi_to_add) => {
if (quasi.length === 0) {
quasi.push(quasi_to_add);
return;
}
quasi[quasi.length - 1] = quasi[quasi.length - 1].concat(quasi_to_add);
},
push_expression: (/** @type {Expression} */ expression_to_add) => {
if (quasi.length === 0) {
quasi.push('');
}
expressions.push(expression_to_add);
quasi.push('');
}
},
locations: [],
transform: { ...context.state.transform },
metadata: {
Expand Down Expand Up @@ -115,7 +135,12 @@ export function Fragment(node, context) {
});

/** @type {Expression[]} */
const args = [b.template([b.quasi(state.template.join(''), true)], [])];
const args = [
b.template(
quasi.map((q) => b.quasi(q, true)),
expressions
)
];

if (state.metadata.context.template_needs_import_node) {
args.push(b.literal(TEMPLATE_USE_IMPORT_NODE));
Expand Down Expand Up @@ -170,12 +195,15 @@ export function Fragment(node, context) {
flags |= TEMPLATE_USE_IMPORT_NODE;
}

if (state.template.length === 1 && state.template[0] === '<!>') {
if (quasi.length === 1 && quasi[0] === '<!>') {
// special case — we can use `$.comment` instead of creating a unique template
body.push(b.var(id, b.call('$.comment')));
} else {
add_template(template_name, [
b.template([b.quasi(state.template.join(''), true)], []),
b.template(
quasi.map((q) => b.quasi(q, true)),
expressions
),
b.literal(flags)
]);

Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/HtmlTag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as b from '../../../../utils/builders.js';
* @param {ComponentContext} context
*/
export function HtmlTag(node, context) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');

// push into init, so that bindings run afterwards, which might trigger another run and override hydration
context.state.init.push(
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import * as b from '../../../../utils/builders.js';
* @param {ComponentContext} context
*/
export function IfBlock(node, context) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');

const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));

Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/KeyBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import * as b from '../../../../utils/builders.js';
* @param {ComponentContext} context
*/
export function KeyBlock(node, context) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');

const key = /** @type {Expression} */ (context.visit(node.expression));
const body = /** @type {Expression} */ (context.visit(node.fragment));
Expand Down
53 changes: 45 additions & 8 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ 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 } from '../utils.js';
import { build_getter, can_inline_variable } from '../utils.js';
import {
get_attribute_name,
build_attribute_value,
Expand Down Expand Up @@ -53,7 +53,7 @@ export function RegularElement(node, context) {
}

if (node.name === 'noscript') {
context.state.template.push('<noscript></noscript>');
context.state.template.push_quasi('<noscript></noscript>');
return;
}

Expand All @@ -67,7 +67,7 @@ export function RegularElement(node, context) {
namespace: determine_namespace_for_children(node, context.state.metadata.namespace)
};

context.state.template.push(`<${node.name}`);
context.state.template.push_quasi(`<${node.name}`);

/** @type {Array<AST.Attribute | AST.SpreadAttribute>} */
const attributes = [];
Expand Down Expand Up @@ -241,7 +241,7 @@ export function RegularElement(node, context) {
const value = is_text_attribute(attribute) ? attribute.value[0].data : true;

if (name !== 'class' || value) {
context.state.template.push(
context.state.template.push_quasi(
` ${attribute.name}${
is_boolean_attribute(name) && value === true
? ''
Expand Down Expand Up @@ -278,7 +278,7 @@ export function RegularElement(node, context) {
context.state.after_update.push(b.stmt(b.call('$.replay_events', node_id)));
}

context.state.template.push('>');
context.state.template.push_quasi('>');

/** @type {SourceLocation[]} */
const child_locations = [];
Expand Down Expand Up @@ -377,7 +377,7 @@ export function RegularElement(node, context) {
}

if (!is_void(node.name)) {
context.state.template.push(`</${node.name}>`);
context.state.template.push_quasi(`</${node.name}>`);
}
}

Expand Down Expand Up @@ -465,7 +465,7 @@ function build_element_spread_attributes(
value.type === 'Literal' &&
context.state.metadata.namespace === 'html'
) {
context.state.template.push(` is="${escape_html(value.value, true)}"`);
context.state.template.push_quasi(` is="${escape_html(value.value, true)}"`);
continue;
}

Expand Down Expand Up @@ -607,6 +607,13 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
);
}

const inlinable_expression =
attribute.value === true
benmccann marked this conversation as resolved.
Show resolved Hide resolved
? false // not an expression
: is_inlinable_expression(
Array.isArray(attribute.value) ? attribute.value : [attribute.value],
context.state
);
if (attribute.metadata.expression.has_state) {
if (has_call) {
state.init.push(build_update(update));
Expand All @@ -615,11 +622,41 @@ function build_element_attribute_update_assignment(element, node_id, attribute,
}
return true;
} else {
state.init.push(update);
if (inlinable_expression) {
context.state.template.push_quasi(` ${name}="`);
context.state.template.push_expression(value);
context.state.template.push_quasi('"');
} else {
state.init.push(update);
}
return false;
}
}

/**
* @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
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/RenderTag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as b from '../../../../utils/builders.js';
* @param {ComponentContext} context
*/
export function RenderTag(node, context) {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');
const callee = unwrap_optional(node.expression).callee;
const raw_args = unwrap_optional(node.expression).arguments;

Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { build_attribute_value } from './shared/element.js';
*/
export function SlotElement(node, context) {
// <slot {a}>fallback</slot> --> $.slot($$slots.default, { get a() { .. } }, () => ...fallback);
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');

/** @type {Property[]} */
const props = [];
Expand Down
2 changes: 1 addition & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { build_render_statement, build_update } from './shared/utils.js';
* @param {ComponentContext} context
*/
export function SvelteElement(node, context) {
context.state.template.push(`<!>`);
context.state.template.push_quasi(`<!>`);

/** @type {Array<AST.Attribute | AST.SpreadAttribute>} */
const attributes = [];
Expand Down
4 changes: 2 additions & 2 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ export function build_component(node, component_name, context, anchor = context.
}

if (Object.keys(custom_css_props).length > 0) {
context.state.template.push(
context.state.template.push_quasi(
context.state.metadata.namespace === 'svg'
? '<g><!></g>'
: '<div style="display: contents"><!></div>'
Expand All @@ -369,7 +369,7 @@ export function build_component(node, component_name, context, anchor = context.
b.stmt(b.call('$.reset', anchor))
);
} else {
context.state.template.push('<!>');
context.state.template.push_quasi('<!>');
statements.push(b.stmt(fn(anchor)));
}

Expand Down
4 changes: 2 additions & 2 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ export function process_children(nodes, initial, is_element, { visit, state }) {
function flush_sequence(sequence) {
if (sequence.every((node) => node.type === 'Text')) {
skipped += 1;
state.template.push(sequence.map((node) => node.raw).join(''));
state.template.push_quasi(sequence.map((node) => node.raw).join(''));
return;
}

state.template.push(' ');
state.template.push_quasi(' ');

const { has_state, has_call, value } = build_template_literal(sequence, visit, state);

Expand Down
19 changes: 19 additions & 0 deletions packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/client/index.svelte.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import "svelte/internal/disclose-version";
import * as $ from "svelte/internal/client";

const __DECLARED_ASSET_0__ = "__VITE_ASSET__2AM7_y_a__ 1440w, __VITE_ASSET__2AM7_y_b__ 960w";
Copy link
Member

Choose a reason for hiding this comment

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

are these later inlined completely? it should be possible to have one static string here instead of interpolation

Copy link
Member Author

@benmccann benmccann Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, but that's a bit more complicated to handle here and we've generally tried to avoid adding such complication here in favor of letting general purpose optimizers handle it. Unfortunately esbuild does not yet handle this case, but I've filed a feature request for it: evanw/esbuild#3570. Oxc also does not handle it, so I've file a request there as well so that it will be handled when switching to rolldown, but it was unfortunately closed for the time being: oxc-project/oxc#2646

Copy link
Member

Choose a reason for hiding this comment

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

so in lieu of external tools supporting it, do we reconsider inlining these ourselves? it would make svelte output more svelte after all.

Copy link
Member Author

Choose a reason for hiding this comment

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

potentially as a future enhancement if folks agree. i'm still hoping esbuild and oxc will add support. they haven't said they're against it, but just haven't prioritized it. I'd rather not try to do it as part of this PR though to keep things simple for now as this solves 95% of the issue

const __DECLARED_ASSET_1__ = "__VITE_ASSET__2AM7_y_c__ 1440w, __VITE_ASSET__2AM7_y_d__ 960w";
const __DECLARED_ASSET_2__ = "__VITE_ASSET__2AM7_y_e__ 1440w, __VITE_ASSET__2AM7_y_f__ 960w";
const __DECLARED_ASSET_3__ = "__VITE_ASSET__2AM7_y_g__";
var root = $.template(`<picture><source srcset="${__DECLARED_ASSET_0__}" type="image/avif"> <source srcset="${__DECLARED_ASSET_1__}" type="image/webp"> <source srcset="${__DECLARED_ASSET_2__}" type="image/png"> <img src="${__DECLARED_ASSET_3__}" alt="production test" width="1440" height="1440"></picture>`);
benmccann marked this conversation as resolved.
Show resolved Hide resolved

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);

$.reset(picture);
$.append($$anchor, picture);
}
10 changes: 10 additions & 0 deletions packages/svelte/tests/snapshot/samples/inline-module-vars/_expected/server/index.svelte.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as $ from "svelte/internal/server";

const __DECLARED_ASSET_0__ = "__VITE_ASSET__2AM7_y_a__ 1440w, __VITE_ASSET__2AM7_y_b__ 960w";
const __DECLARED_ASSET_1__ = "__VITE_ASSET__2AM7_y_c__ 1440w, __VITE_ASSET__2AM7_y_d__ 960w";
const __DECLARED_ASSET_2__ = "__VITE_ASSET__2AM7_y_e__ 1440w, __VITE_ASSET__2AM7_y_f__ 960w";
const __DECLARED_ASSET_3__ = "__VITE_ASSET__2AM7_y_g__";

export default function Inline_module_vars($$payload) {
$$payload.out += `<picture><source${$.attr("srcset", __DECLARED_ASSET_0__)} type="image/avif"> <source${$.attr("srcset", __DECLARED_ASSET_1__)} type="image/webp"> <source${$.attr("srcset", __DECLARED_ASSET_2__)} type="image/png"> <img${$.attr("src", __DECLARED_ASSET_3__)} alt="production test" width="1440" height="1440"></picture>`;
}
15 changes: 15 additions & 0 deletions packages/svelte/tests/snapshot/samples/inline-module-vars/index.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<svelte:options runes={true} />

<script module>
const __DECLARED_ASSET_0__ = "__VITE_ASSET__2AM7_y_a__ 1440w, __VITE_ASSET__2AM7_y_b__ 960w";
const __DECLARED_ASSET_1__ = "__VITE_ASSET__2AM7_y_c__ 1440w, __VITE_ASSET__2AM7_y_d__ 960w";
const __DECLARED_ASSET_2__ = "__VITE_ASSET__2AM7_y_e__ 1440w, __VITE_ASSET__2AM7_y_f__ 960w";
const __DECLARED_ASSET_3__ = "__VITE_ASSET__2AM7_y_g__";
</script>

<picture>
<source srcset={__DECLARED_ASSET_0__} type="image/avif" />
<source srcset={__DECLARED_ASSET_1__} type="image/webp" />
<source srcset={__DECLARED_ASSET_2__} type="image/png" />
<img src={__DECLARED_ASSET_3__} alt="production test" width=1440 height=1440 />
</picture>