From b49cd79bc4c5064a0ddd34832670029ceebb6997 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 23 Jul 2024 13:08:57 +0100 Subject: [PATCH 1/6] fix: ensure dynamic event handlers are wrapped in a derived --- .changeset/bright-colts-play.md | 5 +++++ .../3-transform/client/visitors/template.js | 13 ++++++++++--- .../dynamic-element-event-handler3/_config.js | 15 +++++++++++++++ .../dynamic-element-event-handler3/main.svelte | 9 +++++++++ 4 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 .changeset/bright-colts-play.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/main.svelte diff --git a/.changeset/bright-colts-play.md b/.changeset/bright-colts-play.md new file mode 100644 index 000000000000..758264da1020 --- /dev/null +++ b/.changeset/bright-colts-play.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure dynamic event handlers are wrapped in a derived diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index af10f0a4cd6f..9099ad2aed5e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1132,20 +1132,27 @@ function serialize_event_handler(node, { state, visit }) { handler = node.expression; // Event handlers can be dynamic (source/store/prop/conditional etc) - const dynamic_handler = () => - b.function( + const dynamic_handler = () => { + const id = b.id(state.scope.generate('event_handler')); + + state.init.push( + b.var(id, b.call('$.derived', b.thunk(/** @type {Expression} */ (visit(handler))))) + ); + + return b.function( null, [b.rest(b.id('$$args'))], b.block([ b.return( b.call( - b.member(/** @type {Expression} */ (visit(handler)), b.id('apply'), false, true), + b.member(b.call('$.get', id), b.id('apply'), false, true), b.this, b.id('$$args') ) ) ]) ); + }; if (handler.type === 'Identifier' || handler.type === 'MemberExpression') { const id = object(handler); diff --git a/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/_config.js b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/_config.js new file mode 100644 index 000000000000..01c02087f779 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/_config.js @@ -0,0 +1,15 @@ +import { test } from '../../test'; + +export default test({ + html: '', + + test({ assert, logs, target }) { + const button = target.querySelector('button'); + + button?.click(); + button?.click(); + button?.click(); + + assert.deepEqual(logs, ['create', 'trigger', 'trigger', 'trigger']); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/main.svelte b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/main.svelte new file mode 100644 index 000000000000..38d088503999 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/dynamic-element-event-handler3/main.svelte @@ -0,0 +1,9 @@ + + + From b4202bc6366708460e5f8e3d1538d581affc3f61 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 23 Jul 2024 13:35:14 +0100 Subject: [PATCH 2/6] fix test --- .../3-transform/client/visitors/template.js | 3 ++- .../samples/event-attribute-import/_config.js | 18 +++++++-------- .../samples/event-attribute-import/event.js | 14 ------------ .../event-attribute-import/event.svelte | 22 +++++++++++++++++++ .../event-attribute-import/main.svelte | 3 ++- 5 files changed, 34 insertions(+), 26 deletions(-) delete mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.js create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 9099ad2aed5e..8e8945019e7a 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1154,7 +1154,7 @@ function serialize_event_handler(node, { state, visit }) { ); }; - if (handler.type === 'Identifier' || handler.type === 'MemberExpression') { + if (handler.type === 'Identifier') { const id = object(handler); const binding = id === null ? null : state.scope.get(id.name); if ( @@ -1173,6 +1173,7 @@ function serialize_event_handler(node, { state, visit }) { handler = /** @type {Expression} */ (visit(handler)); } } else if ( + handler.type === 'MemberExpression' || handler.type === 'CallExpression' || handler.type === 'ConditionalExpression' || handler.type === 'LogicalExpression' diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/_config.js b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/_config.js index 66bda3def6ce..957adf163f6f 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/_config.js @@ -1,20 +1,18 @@ import { test } from '../../test'; -import { log, handler, log_a } from './event.js'; export default test({ - before_test() { - log.length = 0; - handler.value = log_a; - }, - - async test({ assert, target }) { - const [b1, b2] = target.querySelectorAll('button'); + async test({ assert, logs, target, component }) { + const [b1, b2, b3] = target.querySelectorAll('button'); b1?.click(); - assert.deepEqual(log, ['a']); + assert.deepEqual(logs, ['a']); b2?.click(); b1?.click(); - assert.deepEqual(log, ['a', 'b']); + + b3?.click(); + b1?.click(); + + assert.deepEqual(logs, ['a', 'b', 'a']); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.js b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.js deleted file mode 100644 index 978f672d3074..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.js +++ /dev/null @@ -1,14 +0,0 @@ -/** @type {any[]} */ -export const log = []; - -export const log_a = () => { - log.push('a'); -}; - -export const log_b = () => { - log.push('b'); -}; - -export const handler = { - value: log_a -}; diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte new file mode 100644 index 000000000000..0aa7bab6fb45 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte @@ -0,0 +1,22 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte index 1b743653a7b2..59602c659b52 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte @@ -1,6 +1,7 @@ + From 024e09933ee398a704b6d5148b009a7d82fcabb6 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 23 Jul 2024 14:58:04 +0100 Subject: [PATCH 3/6] feedback --- .../compiler/phases/3-transform/client/visitors/template.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 8e8945019e7a..9099ad2aed5e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1154,7 +1154,7 @@ function serialize_event_handler(node, { state, visit }) { ); }; - if (handler.type === 'Identifier') { + if (handler.type === 'Identifier' || handler.type === 'MemberExpression') { const id = object(handler); const binding = id === null ? null : state.scope.get(id.name); if ( @@ -1173,7 +1173,6 @@ function serialize_event_handler(node, { state, visit }) { handler = /** @type {Expression} */ (visit(handler)); } } else if ( - handler.type === 'MemberExpression' || handler.type === 'CallExpression' || handler.type === 'ConditionalExpression' || handler.type === 'LogicalExpression' From 2d4fb36c409640174629e32c455c9e704b6c5ff7 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 23 Jul 2024 15:49:48 +0100 Subject: [PATCH 4/6] more feedback --- .../3-transform/client/visitors/template.js | 25 ++++++++++++++----- 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 9099ad2aed5e..b680634aa06e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -1132,7 +1132,22 @@ function serialize_event_handler(node, { state, visit }) { handler = node.expression; // Event handlers can be dynamic (source/store/prop/conditional etc) - const dynamic_handler = () => { + const dynamic_handler = () => + b.function( + null, + [b.rest(b.id('$$args'))], + b.block([ + b.return( + b.call( + b.member(/** @type {Expression} */ (visit(handler)), b.id('apply'), false, true), + b.this, + b.id('$$args') + ) + ) + ]) + ); + + const derived_dynamic_handler = () => { const id = b.id(state.scope.generate('event_handler')); state.init.push( @@ -1172,11 +1187,9 @@ function serialize_event_handler(node, { state, visit }) { } else { handler = /** @type {Expression} */ (visit(handler)); } - } else if ( - handler.type === 'CallExpression' || - handler.type === 'ConditionalExpression' || - handler.type === 'LogicalExpression' - ) { + } else if (handler.type === 'CallExpression') { + handler = derived_dynamic_handler(); + } else if (handler.type === 'ConditionalExpression' || handler.type === 'LogicalExpression') { handler = dynamic_handler(); } else { handler = /** @type {Expression} */ (visit(handler)); From 312b1a0a40ef47213b89f82d3c1e28da646c0fe2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 23 Jul 2024 16:26:53 +0100 Subject: [PATCH 5/6] address feedback --- .../3-transform/client/visitors/template.js | 73 +++++++++++-------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index b680634aa06e..a1f231398ba8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -714,7 +714,7 @@ function serialize_inline_component(node, component_name, context, anchor = cont lets.push(/** @type {ExpressionStatement} */ (context.visit(attribute))); } else if (attribute.type === 'OnDirective') { events[attribute.name] ||= []; - let handler = serialize_event_handler(attribute, context); + let handler = serialize_event_handler(attribute, null, context); if (attribute.modifiers.includes('once')) { handler = b.call('$.once', handler); } @@ -1122,9 +1122,10 @@ function serialize_render_stmt(update) { /** * Serializes the event handler function of the `on:` directive * @param {Pick} node + * @param {null | { contains_call_expression: boolean; dynamic: boolean; } | null} metadata * @param {import('../types.js').ComponentContext} context */ -function serialize_event_handler(node, { state, visit }) { +function serialize_event_handler(node, metadata, { state, visit }) { /** @type {Expression} */ let handler; @@ -1147,28 +1148,6 @@ function serialize_event_handler(node, { state, visit }) { ]) ); - const derived_dynamic_handler = () => { - const id = b.id(state.scope.generate('event_handler')); - - state.init.push( - b.var(id, b.call('$.derived', b.thunk(/** @type {Expression} */ (visit(handler))))) - ); - - return b.function( - null, - [b.rest(b.id('$$args'))], - b.block([ - b.return( - b.call( - b.member(b.call('$.get', id), b.id('apply'), false, true), - b.this, - b.id('$$args') - ) - ) - ]) - ); - }; - if (handler.type === 'Identifier' || handler.type === 'MemberExpression') { const id = object(handler); const binding = id === null ? null : state.scope.get(id.name); @@ -1187,8 +1166,33 @@ function serialize_event_handler(node, { state, visit }) { } else { handler = /** @type {Expression} */ (visit(handler)); } - } else if (handler.type === 'CallExpression') { - handler = derived_dynamic_handler(); + } else if ( + metadata?.contains_call_expression && + !( + (handler.type === 'ArrowFunctionExpression' || handler.type === 'FunctionExpression') && + handler.metadata.hoistable + ) + ) { + // Create a derived dynamic event handler + const id = b.id(state.scope.generate('event_handler')); + + state.init.push( + b.var(id, b.call('$.derived', b.thunk(/** @type {Expression} */ (visit(handler))))) + ); + + handler = b.function( + null, + [b.rest(b.id('$$args'))], + b.block([ + b.return( + b.call( + b.member(b.call('$.get', id), b.id('apply'), false, true), + b.this, + b.id('$$args') + ) + ) + ]) + ); } else if (handler.type === 'ConditionalExpression' || handler.type === 'LogicalExpression') { handler = dynamic_handler(); } else { @@ -1226,17 +1230,18 @@ function serialize_event_handler(node, { state, visit }) { /** * Serializes an event handler function of the `on:` directive or an attribute starting with `on` - * @param {{name: string; modifiers: string[]; expression: Expression | null; delegated?: import('#compiler').DelegatedEvent | null; }} node + * @param {{name: string;modifiers: string[];expression: Expression | null;delegated?: import('#compiler').DelegatedEvent | null;}} node + * @param {null | { contains_call_expression: boolean; dynamic: boolean; }} metadata * @param {import('../types.js').ComponentContext} context */ -function serialize_event(node, context) { +function serialize_event(node, metadata, context) { const state = context.state; /** @type {Statement} */ let statement; if (node.expression) { - let handler = serialize_event_handler(node, context); + let handler = serialize_event_handler(node, metadata, context); const event_name = node.name; const delegated = node.delegated; @@ -1305,7 +1310,12 @@ function serialize_event(node, context) { statement = b.stmt(b.call('$.event', ...args)); } else { statement = b.stmt( - b.call('$.event', b.literal(node.name), state.node, serialize_event_handler(node, context)) + b.call( + '$.event', + b.literal(node.name), + state.node, + serialize_event_handler(node, metadata, context) + ) ); } @@ -1343,6 +1353,7 @@ function serialize_event_attribute(node, context) { modifiers, delegated: node.metadata.delegated }, + !Array.isArray(node.value) && node.value?.type === 'ExpressionTag' ? node.value.metadata : null, context ); } @@ -2817,7 +2828,7 @@ export const template_visitors = { context.next({ ...context.state, in_constructor: false }); }, OnDirective(node, context) { - serialize_event(node, context); + serialize_event(node, null, context); }, UseDirective(node, { state, next, visit }) { const params = [b.id('$$node')]; From cb728f062eb79df92526225f66515772369e86da Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 23 Jul 2024 15:18:45 -0400 Subject: [PATCH 6/6] we have .svelte.js files --- .../event-attribute-import/event.svelte | 22 ------------------- .../event-attribute-import/event.svelte.js | 18 +++++++++++++++ .../event-attribute-import/main.svelte | 6 ++--- 3 files changed, 21 insertions(+), 25 deletions(-) delete mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte.js diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte deleted file mode 100644 index 0aa7bab6fb45..000000000000 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte +++ /dev/null @@ -1,22 +0,0 @@ - diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte.js b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte.js new file mode 100644 index 000000000000..3f0f41995ba9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/event.svelte.js @@ -0,0 +1,18 @@ +export const log_a = () => { + console.log('a'); +}; + +export const log_b = () => { + console.log('b'); +}; + +let handle = $state(log_a); + +export const handler = { + get value() { + return handle; + }, + set value(v) { + handle = v; + } +}; diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte index 59602c659b52..ff07e70a0689 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-import/main.svelte @@ -1,7 +1,7 @@ - - + +