diff --git a/.changeset/light-panthers-sell.md b/.changeset/light-panthers-sell.md new file mode 100644 index 0000000000..f4c97363dd --- /dev/null +++ b/.changeset/light-panthers-sell.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +chore: optimize inline new class expressions via hoisting the class diff --git a/packages/svelte/messages/compile-warnings/script.md b/packages/svelte/messages/compile-warnings/script.md index af8b635bb6..045ce2b79c 100644 --- a/packages/svelte/messages/compile-warnings/script.md +++ b/packages/svelte/messages/compile-warnings/script.md @@ -12,7 +12,7 @@ ## perf_avoid_inline_class -> Avoid 'new class' — instead, declare the class at the top level scope +> Avoid complex 'new class' expressions that extend other classes or contain arguemtns within their constructor — instead, declare the class at the top level scope or simplify the `new class` expression ## perf_avoid_nested_class diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 473141bf3e..dc63d6e9c2 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -32,6 +32,7 @@ import { import { Scope, get_rune } from '../scope.js'; import { merge } from '../visitors.js'; import { a11y_validators } from './a11y.js'; +import { can_hoist_inline_class_expression } from '../3-transform/utils.js'; /** @param {import('#compiler').Attribute} attribute */ function validate_attribute(attribute) { @@ -329,6 +330,102 @@ function validate_block_not_empty(node, context) { } } +/** + * @type {import('zimmerframe').Visitors} + */ +export const validation_runes_js = { + ImportDeclaration(node) { + if (typeof node.source.value === 'string' && node.source.value.startsWith('svelte/internal')) { + e.import_svelte_internal_forbidden(node); + } + }, + ExportSpecifier(node, { state }) { + validate_export(node, state.scope, node.local.name); + }, + ExportNamedDeclaration(node, { state, next }) { + if (node.declaration?.type !== 'VariableDeclaration') return; + + // visit children, so bindings are correctly initialised + next(); + + for (const declarator of node.declaration.declarations) { + for (const id of extract_identifiers(declarator.id)) { + validate_export(node, state.scope, id.name); + } + } + }, + CallExpression(node, { state, path }) { + if (get_rune(node, state.scope) === '$host') { + e.host_invalid_placement(node); + } + validate_call_expression(node, state.scope, path); + }, + VariableDeclarator(node, { state }) { + const init = node.init; + const rune = get_rune(init, state.scope); + + if (rune === null) return; + + const args = /** @type {import('estree').CallExpression} */ (init).arguments; + + if ((rune === '$derived' || rune === '$derived.by') && args.length !== 1) { + e.rune_invalid_arguments_length(node, rune, 'exactly one argument'); + } else if (rune === '$state' && args.length > 1) { + e.rune_invalid_arguments_length(node, rune, 'zero or one arguments'); + } else if (rune === '$props') { + e.props_invalid_placement(node); + } else if (rune === '$bindable') { + e.bindable_invalid_location(node); + } + }, + AssignmentExpression(node, { state }) { + validate_assignment(node, node.left, state); + }, + UpdateExpression(node, { state }) { + validate_assignment(node, node.argument, state); + }, + ClassBody(node, context) { + /** @type {string[]} */ + const private_derived_state = []; + + for (const definition of node.body) { + if ( + definition.type === 'PropertyDefinition' && + definition.key.type === 'PrivateIdentifier' && + definition.value?.type === 'CallExpression' + ) { + const rune = get_rune(definition.value, context.state.scope); + if (rune === '$derived' || rune === '$derived.by') { + private_derived_state.push(definition.key.name); + } + } + } + + context.next({ + ...context.state, + private_derived_state + }); + }, + ClassDeclaration(node, context) { + // In modules, we allow top-level module scope only, in components, we allow the component scope, + // which is function_depth of 1. With the exception of `new class` which is also not allowed at + // component scope level either. + const allowed_depth = context.state.ast_type === 'module' ? 0 : 1; + + if (context.state.scope.function_depth > allowed_depth) { + w.perf_avoid_nested_class(node); + } + }, + NewExpression(node, context) { + if ( + node.callee.type === 'ClassExpression' && + !can_hoist_inline_class_expression(node, context.state.ast_type, context.state.scope) + ) { + w.perf_avoid_inline_class(node); + } + } +}; + /** * @type {import('zimmerframe').Visitors} */ @@ -807,7 +904,8 @@ export const validation_legacy = merge(validation, a11y_validators, { }, UpdateExpression(node, { state }) { validate_assignment(node, node.argument, state); - } + }, + NewExpression: validation_runes_js.NewExpression }); /** @@ -933,99 +1031,6 @@ function ensure_no_module_import_conflict(node, state) { } } -/** - * @type {import('zimmerframe').Visitors} - */ -export const validation_runes_js = { - ImportDeclaration(node) { - if (typeof node.source.value === 'string' && node.source.value.startsWith('svelte/internal')) { - e.import_svelte_internal_forbidden(node); - } - }, - ExportSpecifier(node, { state }) { - validate_export(node, state.scope, node.local.name); - }, - ExportNamedDeclaration(node, { state, next }) { - if (node.declaration?.type !== 'VariableDeclaration') return; - - // visit children, so bindings are correctly initialised - next(); - - for (const declarator of node.declaration.declarations) { - for (const id of extract_identifiers(declarator.id)) { - validate_export(node, state.scope, id.name); - } - } - }, - CallExpression(node, { state, path }) { - if (get_rune(node, state.scope) === '$host') { - e.host_invalid_placement(node); - } - validate_call_expression(node, state.scope, path); - }, - VariableDeclarator(node, { state }) { - const init = node.init; - const rune = get_rune(init, state.scope); - - if (rune === null) return; - - const args = /** @type {import('estree').CallExpression} */ (init).arguments; - - if ((rune === '$derived' || rune === '$derived.by') && args.length !== 1) { - e.rune_invalid_arguments_length(node, rune, 'exactly one argument'); - } else if (rune === '$state' && args.length > 1) { - e.rune_invalid_arguments_length(node, rune, 'zero or one arguments'); - } else if (rune === '$props') { - e.props_invalid_placement(node); - } else if (rune === '$bindable') { - e.bindable_invalid_location(node); - } - }, - AssignmentExpression(node, { state }) { - validate_assignment(node, node.left, state); - }, - UpdateExpression(node, { state }) { - validate_assignment(node, node.argument, state); - }, - ClassBody(node, context) { - /** @type {string[]} */ - const private_derived_state = []; - - for (const definition of node.body) { - if ( - definition.type === 'PropertyDefinition' && - definition.key.type === 'PrivateIdentifier' && - definition.value?.type === 'CallExpression' - ) { - const rune = get_rune(definition.value, context.state.scope); - if (rune === '$derived' || rune === '$derived.by') { - private_derived_state.push(definition.key.name); - } - } - } - - context.next({ - ...context.state, - private_derived_state - }); - }, - ClassDeclaration(node, context) { - // In modules, we allow top-level module scope only, in components, we allow the component scope, - // which is function_depth of 1. With the exception of `new class` which is also not allowed at - // component scope level either. - const allowed_depth = context.state.ast_type === 'module' ? 0 : 1; - - if (context.state.scope.function_depth > allowed_depth) { - w.perf_avoid_nested_class(node); - } - }, - NewExpression(node, context) { - if (node.callee.type === 'ClassExpression' && context.state.scope.function_depth > 0) { - w.perf_avoid_inline_class(node); - } - } -}; - /** * @param {import('../../errors.js').NodeLike} node * @param {import('estree').Pattern | import('estree').Expression} argument diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js index da2502b6f5..e589852709 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js @@ -44,6 +44,7 @@ export function client_component(source, analysis, options) { const state = { analysis, options, + ast_type: 'template', scope: analysis.module.scope, scopes: analysis.template.scopes, hoisted: [b.import_all('$', 'svelte/internal/client')], @@ -543,8 +544,10 @@ export function client_module(analysis, options) { const state = { analysis, options, + ast_type: 'module', scope: analysis.module.scope, scopes: analysis.module.scopes, + hoisted: [b.import_all('$', 'svelte/internal/client')], legacy_reactive_statements: new Map(), public_state: new Map(), private_state: new Map(), @@ -568,6 +571,6 @@ export function client_module(analysis, options) { return { type: 'Program', sourceType: 'module', - body: [b.import_all('$', 'svelte/internal/client'), ...module.body] + body: [...state.hoisted, ...module.body] }; } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts index aec2edb68d..136e0d9020 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts @@ -8,11 +8,11 @@ import type { import type { Namespace, SvelteNode, ValidatedCompileOptions } from '#compiler'; import type { TransformState } from '../types.js'; import type { ComponentAnalysis } from '../../types.js'; -import type { Location } from 'locate-character'; export interface ClientTransformState extends TransformState { readonly private_state: Map; readonly public_state: Map; + readonly hoisted: Array; /** * `true` if the current lexical scope belongs to a class constructor. this allows @@ -22,6 +22,8 @@ export interface ClientTransformState extends TransformState { /** The $: calls, which will be ordered in the end */ readonly legacy_reactive_statements: Map; + + readonly ast_type: 'module' | 'template'; } export type SourceLocation = diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js index e08bb6c8ac..fbfb0f1758 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js @@ -1,5 +1,9 @@ -import { get_rune } from '../../../scope.js'; -import { is_hoistable_function, transform_inspect_rune } from '../../utils.js'; +import { get_rune, set_scope } from '../../../scope.js'; +import { + can_hoist_inline_class_expression, + is_hoistable_function, + transform_inspect_rune +} from '../../utils.js'; import * as b from '../../../../utils/builders.js'; import * as assert from '../../../../utils/assert.js'; import { @@ -10,6 +14,8 @@ import { } from '../utils.js'; import { extract_paths } from '../../../../utils/ast.js'; import { regex_invalid_identifier_chars } from '../../../patterns.js'; +import { walk } from 'zimmerframe'; +import is_reference from 'is-reference'; /** @type {import('../types.js').ComponentVisitors} */ export const javascript_visitors_runes = { @@ -490,6 +496,139 @@ export const javascript_visitors_runes = { } next(); + }, + NewExpression(node, context) { + const state = context.state; + if ( + node.callee.type === 'ClassExpression' && + can_hoist_inline_class_expression(node, state.ast_type, state.scope) + ) { + const id = node.callee.id?.name || state.scope.generate('hoisted_inline_class'); + + const { _ } = set_scope(state.scopes); + const closure_identifiers = new Map(); + + let class_expression = /** @type {import('estree').ClassExpression} */ ( + context.visit(node.callee) + ); + + class_expression = /** @type {import('estree').ClassExpression} */ ( + walk(class_expression, state, { + _, + Identifier(node, context) { + const parent = /** @type {import('estree').Expression} */ (context.path.at(-1)); + + if ( + is_reference(node, parent) && + !get_rune(parent, state.scope) && + parent.type !== 'ClassExpression' + ) { + const binding = context.state.scope.get(node.name); + if (binding !== null && binding.scope.function_depth !== 0) { + let closure_identifier = closure_identifiers.get(node.name); + + if (closure_identifier === undefined) { + // We define the kind as being something that is either reactive, in which case we leave the binding + // alone and simply pass the identiifer. If it's mutated, we box it, otherwise we keep it as standard. + closure_identifier = { + id: state.scope.generate('closure_' + node.name), + kind: binding.kind !== 'normal' ? 'ref' : binding.mutated ? 'boxed' : 'normal' + }; + closure_identifiers.set(node.name, closure_identifier); + } + const member = b.member(b.this, b.private_id(closure_identifier.id)); + return closure_identifier.kind === 'boxed' ? b.member(member, b.id('v')) : member; + } + } + + context.next(); + } + }) + ); + + const class_body = class_expression.body.body; + + /** @type {Array} */ + const body_entries = [...closure_identifiers.values()].map(({ id }) => + b.prop_def(b.private_id(id), null) + ); + const class_definitions = []; + + for (let i = 0; i < class_body.length; i++) { + const entry = class_body[i]; + if (entry.type === 'PropertyDefinition') { + if (entry.value) { + class_definitions.push(entry); + } + if (entry.key.type === 'PrivateIdentifier') { + class_body.splice(i, 1, b.prop_def(entry.key, null)); + } else { + class_body.splice(i, 1); + i--; + } + } + } + + let constructor = /** @type {import('estree').MethodDefinition | undefined} */ ( + class_body.find((e) => e.type === 'MethodDefinition' && e.kind === 'constructor') + ); + + if (!constructor) { + constructor = b.method('constructor', b.id('constructor'), [], []); + body_entries.push(constructor); + } + + constructor.value.params.push(...[...closure_identifiers.values()].map(({ id }) => b.id(id))); + + const constructor_body = constructor.value.body.body; + const assignments = [...closure_identifiers.values()].map(({ id }) => + b.stmt(b.assignment('=', b.member(b.this, b.private_id(id)), b.id(id))) + ); + for (const entry of class_definitions) { + assignments.push( + b.stmt( + b.assignment( + '=', + b.member(b.this, entry.key), + /** @type {import('estree').Expression} */ (entry.value) + ) + ) + ); + } + constructor_body.unshift(...assignments); + + class_body.unshift(...body_entries); + + context.state.hoisted.push(b.class(b.id(id), class_expression)); + + return { + ...node, + arguments: [...closure_identifiers.entries()].map(([name, { kind }]) => { + if (kind === 'ref') { + return b.id(name); + } + const visited = /** @type {import('estree').Identifier} */ (context.visit(b.id(name))); + if (kind === 'boxed') { + return b.object([ + b.prop('get', b.id('v'), b.function(null, [], b.block([b.return(visited)]))), + b.prop( + 'set', + b.id('v'), + b.function( + null, + [b.id('v')], + b.block([b.stmt(b.assignment('=', b.id(name), b.id('v')))]) + ) + ) + ]); + } + return visited; + }), + callee: b.id(id) + }; + } + + context.next(); } }; diff --git a/packages/svelte/src/compiler/phases/3-transform/utils.js b/packages/svelte/src/compiler/phases/3-transform/utils.js index 6299177198..29d052cfe6 100644 --- a/packages/svelte/src/compiler/phases/3-transform/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/utils.js @@ -408,3 +408,58 @@ export function transform_inspect_rune(node, context) { return b.call('$.inspect', as_fn ? b.thunk(b.array(arg)) : b.array(arg)); } } + +/** + * @param {import("estree").NewExpression} node + * @param {string} ast_type + * @param {import("../scope.js").Scope} scope + */ +export function can_hoist_inline_class_expression(node, ast_type, scope) { + const allowed_depth = ast_type === 'module' ? 0 : 1; + + if (node.callee.type !== 'ClassExpression') { + return false; + } + // If the new expression has arguments, skip hoisting it. + if (node.arguments.length > 0) { + return false; + } + const super_class = node.callee.superClass; + if (super_class) { + // We don't support hoisting a class that extends another class + return false; + } + // If the class is top level, do not bother inlining it. + if (scope.function_depth < allowed_depth) { + return false; + } + const body = node.callee.body.body; + + for (const body_entry of body) { + if (body_entry.type === 'MethodDefinition' && body_entry.kind === 'constructor') { + const func = body_entry.value; + // If the constructor has arguments we can't hoist the class as we need to rewrite the arguments. + if (func.params.length > 0) { + return false; + } + let has_arguments_reference = false; + + // If the constructor contains a reference to `arguments` we also can't hoist. + walk(func, null, { + // @ts-ignore + Identifier(node, context) { + const parent = /** @type {import('estree').Expression} */ (context.path.at(-1)); + + if (is_reference(node, parent) && node.name === 'arguments') { + has_arguments_reference = true; + } + } + }); + if (has_arguments_reference) { + return false; + } + } + } + + return true; +} diff --git a/packages/svelte/src/compiler/utils/builders.js b/packages/svelte/src/compiler/utils/builders.js index 6070a96702..f95f88fe44 100644 --- a/packages/svelte/src/compiler/utils/builders.js +++ b/packages/svelte/src/compiler/utils/builders.js @@ -508,6 +508,19 @@ function var_builder(pattern, init) { return declaration('var', pattern, init); } +/** + * @returns {import('estree').ClassDeclaration} + * @param {import("estree").Identifier} id + * @param {import("estree").ClassExpression} entries + */ +function class_builder(id, entries) { + return { + ...entries, + id, + type: 'ClassDeclaration' + }; +} + /** * * @param {import('estree').VariableDeclaration | import('estree').Expression | null} init @@ -624,6 +637,7 @@ export { let_builder as let, const_builder as const, var_builder as var, + class_builder as class, true_instance as true, false_instance as false, for_builder as for,