Skip to content

Commit 4eb432e

Browse files
Rich-HarrisRich-Harris
authored
chore: remove event hoisting (#17030)
* chore: get rid of hoisted event handlers * remove unused stuff * simplify * wow we can delete so much more code. this makes me so happy * even more!
1 parent c08ecba commit 4eb432e

File tree

21 files changed

+45
-483
lines changed

21 files changed

+45
-483
lines changed

‎packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js‎

Lines changed: 5 additions & 183 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,7 @@
1-
/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression } from 'estree' */
2-
/** @import { AST, DelegatedEvent } from '#compiler' */
1+
/** @import { AST } from '#compiler' */
32
/** @import { Context } from '../types' */
4-
import { cannot_be_set_statically, is_capture_event, is_delegated } from '../../../../utils.js';
5-
import {
6-
get_attribute_chunks,
7-
get_attribute_expression,
8-
is_event_attribute
9-
} from '../../../utils/ast.js';
3+
import { cannot_be_set_statically, can_delegate_event } from '../../../../utils.js';
4+
import { get_attribute_chunks, is_event_attribute } from '../../../utils/ast.js';
105
import { mark_subtree_dynamic } from './shared/fragment.js';
116

127
/**
@@ -64,181 +59,8 @@ export function Attribute(node, context) {
6459
context.state.analysis.uses_event_attributes = true;
6560
}
6661

67-
const expression = get_attribute_expression(node);
68-
const delegated_event = get_delegated_event(node.name.slice(2), expression, context);
69-
70-
if (delegated_event !== null) {
71-
if (delegated_event.hoisted) {
72-
delegated_event.function.metadata.hoisted = true;
73-
}
74-
75-
node.metadata.delegated = delegated_event;
76-
}
77-
}
78-
}
79-
}
80-
81-
/** @type {DelegatedEvent} */
82-
const unhoisted = { hoisted: false };
83-
84-
/**
85-
* Checks if given event attribute can be delegated/hoisted and returns the corresponding info if so
86-
* @param {string} event_name
87-
* @param {Expression | null} handler
88-
* @param {Context} context
89-
* @returns {null | DelegatedEvent}
90-
*/
91-
function get_delegated_event(event_name, handler, context) {
92-
// Handle delegated event handlers. Bail out if not a delegated event.
93-
if (!handler || !is_delegated(event_name)) {
94-
return null;
95-
}
96-
97-
// If we are not working with a RegularElement, then bail out.
98-
const element = context.path.at(-1);
99-
if (element?.type !== 'RegularElement') {
100-
return null;
101-
}
102-
103-
/** @type {FunctionExpression | FunctionDeclaration | ArrowFunctionExpression | null} */
104-
let target_function = null;
105-
let binding = null;
106-
107-
if (element.metadata.has_spread) {
108-
// event attribute becomes part of the dynamic spread array
109-
return unhoisted;
110-
}
111-
112-
if (handler.type === 'ArrowFunctionExpression' || handler.type === 'FunctionExpression') {
113-
target_function = handler;
114-
} else if (handler.type === 'Identifier') {
115-
binding = context.state.scope.get(handler.name);
116-
117-
if (context.state.analysis.module.scope.references.has(handler.name)) {
118-
// If a binding with the same name is referenced in the module scope (even if not declared there), bail out
119-
return unhoisted;
120-
}
121-
122-
if (binding != null) {
123-
for (const { path } of binding.references) {
124-
const parent = path.at(-1);
125-
if (parent === undefined) return unhoisted;
126-
127-
const grandparent = path.at(-2);
128-
129-
/** @type {AST.RegularElement | null} */
130-
let element = null;
131-
/** @type {string | null} */
132-
let event_name = null;
133-
if (parent.type === 'OnDirective') {
134-
element = /** @type {AST.RegularElement} */ (grandparent);
135-
event_name = parent.name;
136-
} else if (
137-
parent.type === 'ExpressionTag' &&
138-
grandparent?.type === 'Attribute' &&
139-
is_event_attribute(grandparent)
140-
) {
141-
element = /** @type {AST.RegularElement} */ (path.at(-3));
142-
const attribute = /** @type {AST.Attribute} */ (grandparent);
143-
event_name = get_attribute_event_name(attribute.name);
144-
}
145-
146-
if (element && event_name) {
147-
if (
148-
element.type !== 'RegularElement' ||
149-
element.metadata.has_spread ||
150-
!is_delegated(event_name)
151-
) {
152-
return unhoisted;
153-
}
154-
} else if (parent.type !== 'FunctionDeclaration' && parent.type !== 'VariableDeclarator') {
155-
return unhoisted;
156-
}
157-
}
62+
node.metadata.delegated =
63+
parent?.type === 'RegularElement' && can_delegate_event(node.name.slice(2));
15864
}
159-
160-
// If the binding is exported, bail out
161-
if (context.state.analysis.exports.find((node) => node.name === handler.name)) {
162-
return unhoisted;
163-
}
164-
165-
if (binding?.is_function()) {
166-
target_function = binding.initial;
167-
}
168-
}
169-
170-
// If we can't find a function, or the function has multiple parameters, bail out
171-
if (target_function == null || target_function.params.length > 1) {
172-
return unhoisted;
173-
}
174-
175-
const visited_references = new Set();
176-
const scope = target_function.metadata.scope;
177-
for (const [reference] of scope.references) {
178-
// Bail out if the arguments keyword is used or $host is referenced
179-
if (reference === 'arguments' || reference === '$host') return unhoisted;
180-
// Bail out if references a store subscription
181-
if (scope.get(`$${reference}`)?.kind === 'store_sub') return unhoisted;
182-
183-
const binding = scope.get(reference);
184-
const local_binding = context.state.scope.get(reference);
185-
186-
// if the function access a snippet that can't be hoisted we bail out
187-
if (
188-
local_binding !== null &&
189-
local_binding.initial?.type === 'SnippetBlock' &&
190-
!local_binding.initial.metadata.can_hoist
191-
) {
192-
return unhoisted;
193-
}
194-
195-
// If we are referencing a binding that is shadowed in another scope then bail out (unless it's declared within the function).
196-
if (
197-
local_binding !== null &&
198-
binding !== null &&
199-
local_binding.node !== binding.node &&
200-
scope.declarations.get(reference) !== binding
201-
) {
202-
return unhoisted;
203-
}
204-
205-
// If we have multiple references to the same store using $ prefix, bail out.
206-
if (
207-
binding !== null &&
208-
binding.kind === 'store_sub' &&
209-
visited_references.has(reference.slice(1))
210-
) {
211-
return unhoisted;
212-
}
213-
214-
// If we reference the index within an each block, then bail out.
215-
if (binding !== null && binding.initial?.type === 'EachBlock') return unhoisted;
216-
217-
if (
218-
binding !== null &&
219-
// Bail out if the binding is a rest param
220-
(binding.declaration_kind === 'rest_param' ||
221-
// Bail out if we reference anything from the EachBlock (for now) that mutates in non-runes mode,
222-
(((!context.state.analysis.runes && binding.kind === 'each') ||
223-
// or any normal not reactive bindings that are mutated.
224-
binding.kind === 'normal') &&
225-
binding.updated))
226-
) {
227-
return unhoisted;
228-
}
229-
visited_references.add(reference);
230-
}
231-
232-
return { hoisted: true, function: target_function };
233-
}
234-
235-
/**
236-
* @param {string} event_name
237-
*/
238-
function get_attribute_event_name(event_name) {
239-
event_name = event_name.slice(2);
240-
if (is_capture_event(event_name)) {
241-
event_name = event_name.slice(0, -7);
24265
}
243-
return event_name;
24466
}

‎packages/svelte/src/compiler/phases/2-analyze/visitors/shared/function.js‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,6 @@
66
* @param {Context} context
77
*/
88
export function visit_function(node, context) {
9-
// TODO retire this in favour of a more general solution based on bindings
10-
node.metadata = {
11-
hoisted: false,
12-
hoisted_params: [],
13-
scope: context.state.scope
14-
};
15-
169
if (context.state.expression) {
1710
for (const [name] of context.state.scope.references) {
1811
const binding = context.state.scope.get(name);

‎packages/svelte/src/compiler/phases/3-transform/client/utils.js‎

Lines changed: 2 additions & 124 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
1-
/** @import { ArrowFunctionExpression, AssignmentExpression, BlockStatement, Expression, FunctionDeclaration, FunctionExpression, Identifier, Node, Pattern, UpdateExpression } from 'estree' */
1+
/** @import { BlockStatement, Expression, Identifier } from 'estree' */
22
/** @import { Binding } from '#compiler' */
3-
/** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
3+
/** @import { ClientTransformState, ComponentClientTransformState } from './types.js' */
44
/** @import { Analysis } from '../../types.js' */
55
/** @import { Scope } from '../../scope.js' */
66
import * as b from '#compiler/builders';
@@ -12,9 +12,6 @@ import {
1212
PROPS_IS_UPDATED,
1313
PROPS_IS_BINDABLE
1414
} from '../../../../constants.js';
15-
import { dev } from '../../../state.js';
16-
import { walk } from 'zimmerframe';
17-
import { validate_mutation } from './visitors/shared/utils.js';
1815

1916
/**
2017
* @param {Binding} binding
@@ -46,125 +43,6 @@ export function build_getter(node, state) {
4643
return node;
4744
}
4845

49-
/**
50-
* @param {FunctionDeclaration | FunctionExpression | ArrowFunctionExpression} node
51-
* @param {ComponentContext} context
52-
* @returns {Pattern[]}
53-
*/
54-
function get_hoisted_params(node, context) {
55-
const scope = context.state.scope;
56-
57-
/** @type {Identifier[]} */
58-
const params = [];
59-
60-
/**
61-
* We only want to push if it's not already present to avoid name clashing
62-
* @param {Identifier} id
63-
*/
64-
function push_unique(id) {
65-
if (!params.find((param) => param.name === id.name)) {
66-
params.push(id);
67-
}
68-
}
69-
70-
for (const [reference] of scope.references) {
71-
let binding = scope.get(reference);
72-
73-
if (binding !== null && !scope.declarations.has(reference) && binding.initial !== node) {
74-
if (binding.kind === 'store_sub') {
75-
// We need both the subscription for getting the value and the store for updating
76-
push_unique(b.id(binding.node.name));
77-
binding = /** @type {Binding} */ (scope.get(binding.node.name.slice(1)));
78-
}
79-
80-
let expression = context.state.transform[reference]?.read(b.id(binding.node.name));
81-
82-
if (
83-
// If it's a destructured derived binding, then we can extract the derived signal reference and use that.
84-
// TODO this code is bad, we need to kill it
85-
expression != null &&
86-
typeof expression !== 'function' &&
87-
expression.type === 'MemberExpression' &&
88-
expression.object.type === 'CallExpression' &&
89-
expression.object.callee.type === 'Identifier' &&
90-
expression.object.callee.name === '$.get' &&
91-
expression.object.arguments[0].type === 'Identifier'
92-
) {
93-
push_unique(b.id(expression.object.arguments[0].name));
94-
} else if (
95-
// If we are referencing a simple $$props value, then we need to reference the object property instead
96-
(binding.kind === 'prop' || binding.kind === 'bindable_prop') &&
97-
!is_prop_source(binding, context.state)
98-
) {
99-
push_unique(b.id('$$props'));
100-
} else if (
101-
// imports don't need to be hoisted
102-
binding.declaration_kind !== 'import'
103-
) {
104-
// create a copy to remove start/end tags which would mess up source maps
105-
push_unique(b.id(binding.node.name));
106-
// rest props are often accessed through the $$props object for optimization reasons,
107-
// but we can't know if the delegated event handler will use it, so we need to add both as params
108-
if (binding.kind === 'rest_prop' && context.state.analysis.runes) {
109-
push_unique(b.id('$$props'));
110-
}
111-
}
112-
}
113-
}
114-
115-
if (dev) {
116-
// this is a little hacky, but necessary for ownership validation
117-
// to work inside hoisted event handlers
118-
119-
/**
120-
* @param {AssignmentExpression | UpdateExpression} node
121-
* @param {{ next: () => void, stop: () => void }} context
122-
*/
123-
function visit(node, { next, stop }) {
124-
if (validate_mutation(node, /** @type {any} */ (context), node) !== node) {
125-
params.push(b.id('$$ownership_validator'));
126-
stop();
127-
} else {
128-
next();
129-
}
130-
}
131-
132-
walk(/** @type {Node} */ (node), null, {
133-
AssignmentExpression: visit,
134-
UpdateExpression: visit
135-
});
136-
}
137-
138-
return params;
139-
}
140-
141-
/**
142-
* @param {FunctionDeclaration | FunctionExpression | ArrowFunctionExpression} node
143-
* @param {ComponentContext} context
144-
* @returns {Pattern[]}
145-
*/
146-
export function build_hoisted_params(node, context) {
147-
const hoisted_params = get_hoisted_params(node, context);
148-
node.metadata.hoisted_params = hoisted_params;
149-
150-
/** @type {Pattern[]} */
151-
const params = [];
152-
153-
if (node.params.length === 0) {
154-
if (hoisted_params.length > 0) {
155-
// For the event object
156-
params.push(b.id(context.state.scope.generate('_')));
157-
}
158-
} else {
159-
for (const param of node.params) {
160-
params.push(/** @type {Pattern} */ (context.visit(param)));
161-
}
162-
}
163-
164-
params.push(...hoisted_params);
165-
return params;
166-
}
167-
16846
/**
16947
* @param {Binding} binding
17048
* @param {ComponentClientTransformState} state

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/FunctionDeclaration.js‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
/** @import { FunctionDeclaration } from 'estree' */
22
/** @import { ComponentContext } from '../types' */
3-
import { build_hoisted_params } from '../utils.js';
4-
import * as b from '#compiler/builders';
53

64
/**
75
* @param {FunctionDeclaration} node
@@ -10,14 +8,5 @@ import * as b from '#compiler/builders';
108
export function FunctionDeclaration(node, context) {
119
const state = { ...context.state, in_constructor: false, in_derived: false };
1210

13-
if (node.metadata?.hoisted === true) {
14-
const params = build_hoisted_params(node, context);
15-
const body = context.visit(node.body, state);
16-
17-
context.state.hoisted.push(/** @type {FunctionDeclaration} */ ({ ...node, params, body }));
18-
19-
return b.empty;
20-
}
21-
2211
context.next(state);
2312
}

0 commit comments

Comments
 (0)