From 9716c31347bbccea8057ee18c00935abd207ab11 Mon Sep 17 00:00:00 2001 From: paoloricciuti Date: Sun, 28 Apr 2024 23:57:59 +0200 Subject: [PATCH 1/3] fix: store from props hoist wrong param --- .changeset/tiny-moose-kiss.md | 5 +++++ .../compiler/phases/3-transform/client/utils.js | 16 ++++++++++++++-- .../samples/store-from-props-hoisting/_config.js | 9 +++++++++ .../store-from-props-hoisting/main.svelte | 10 ++++++++++ 4 files changed, 38 insertions(+), 2 deletions(-) create mode 100644 .changeset/tiny-moose-kiss.md create mode 100644 packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte diff --git a/.changeset/tiny-moose-kiss.md b/.changeset/tiny-moose-kiss.md new file mode 100644 index 000000000000..e37fa126a391 --- /dev/null +++ b/.changeset/tiny-moose-kiss.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: store from props hoist wrong param 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 481caf656182..ece7b79aba8b 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -473,8 +473,20 @@ function get_hoistable_params(node, context) { if (binding !== null && !scope.declarations.has(reference) && binding.initial !== node) { if (binding.kind === 'store_sub') { - // We need both the subscription for getting the value and the store for updating - safe_push(b.id(binding.node.name.slice(1))); + const is_from_prop = + binding.declaration_kind === 'synthetic' && + [...binding.scope.declarations.values()].find( + (declaration) => declaration.kind === 'prop' || declaration.kind === 'bindable_prop' + ); + if (is_from_prop && !added_props) { + // if the store come from props we want $$props to be pushed rather than the name + // of the store since that variable doesn't exist + added_props = true; + safe_push(b.id('$$props')); + } else { + // We need both the subscription for getting the value and the store for updating + safe_push(b.id(binding.node.name.slice(1))); + } safe_push(b.id(binding.node.name)); } else if ( // If it's a destructured derived binding, then we can extract the derived signal reference and use that. diff --git a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js new file mode 100644 index 000000000000..e5e25e85a54f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js @@ -0,0 +1,9 @@ +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + recover: true, + mode: ['client'] +}); diff --git a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte new file mode 100644 index 000000000000..ebbf42aa7c1b --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte @@ -0,0 +1,10 @@ + + + From 2f6ff9142acadf45406a189e553469fe652dd8dd Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 29 Apr 2024 09:25:14 +0200 Subject: [PATCH 2/3] simplify --- .../phases/3-transform/client/utils.js | 41 +++++++------------ .../store-from-props-hoisting/_config.js | 13 +++++- .../store-from-props-hoisting/child.svelte | 8 ++++ .../store-from-props-hoisting/main.svelte | 11 ++--- 4 files changed, 37 insertions(+), 36 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/child.svelte 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 ece7b79aba8b..e6fdb902f0fe 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -456,39 +456,30 @@ function get_hoistable_params(node, context) { /** @type {import('estree').Identifier[]} */ const params = []; - let added_props = false; /** - * we only want to push if it's not already present to avoid name clashing + * We only want to push if it's not already present to avoid name clashing * @param {import('estree').Identifier} id */ - function safe_push(id) { + function push_unique(id) { if (!params.find((param) => param.name === id.name)) { params.push(id); } } for (const [reference] of scope.references) { - const binding = scope.get(reference); + let binding = scope.get(reference); if (binding !== null && !scope.declarations.has(reference) && binding.initial !== node) { if (binding.kind === 'store_sub') { - const is_from_prop = - binding.declaration_kind === 'synthetic' && - [...binding.scope.declarations.values()].find( - (declaration) => declaration.kind === 'prop' || declaration.kind === 'bindable_prop' - ); - if (is_from_prop && !added_props) { - // if the store come from props we want $$props to be pushed rather than the name - // of the store since that variable doesn't exist - added_props = true; - safe_push(b.id('$$props')); - } else { - // We need both the subscription for getting the value and the store for updating - safe_push(b.id(binding.node.name.slice(1))); - } - safe_push(b.id(binding.node.name)); - } else if ( + // We need both the subscription for getting the value and the store for updating + push_unique(b.id(binding.node.name)); + binding = /** @type {import('#compiler').Binding} */ ( + scope.get(binding.node.name.slice(1)) + ); + } + + if ( // If it's a destructured derived binding, then we can extract the derived signal reference and use that. binding.expression !== null && typeof binding.expression !== 'function' && @@ -498,7 +489,7 @@ function get_hoistable_params(node, context) { binding.expression.object.callee.name === '$.get' && binding.expression.object.arguments[0].type === 'Identifier' ) { - safe_push(b.id(binding.expression.object.arguments[0].name)); + push_unique(b.id(binding.expression.object.arguments[0].name)); } else if ( // If we are referencing a simple $$props value, then we need to reference the object property instead (binding.kind === 'prop' || binding.kind === 'bindable_prop') && @@ -506,14 +497,10 @@ function get_hoistable_params(node, context) { binding.initial === null && !context.state.analysis.accessors ) { - // Handle $$props.something use-cases - if (!added_props) { - added_props = true; - safe_push(b.id('$$props')); - } + push_unique(b.id('$$props')); } else { // create a copy to remove start/end tags which would mess up source maps - safe_push(b.id(binding.node.name)); + push_unique(b.id(binding.node.name)); } } } diff --git a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js index e5e25e85a54f..bd5593eb0863 100644 --- a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/_config.js @@ -4,6 +4,15 @@ export default test({ compileOptions: { dev: true }, - recover: true, - mode: ['client'] + async test({ assert, target }) { + const button = target.querySelector('button'); + await button?.click(); + + assert.htmlEqual( + target.innerHTML, + ` + + ` + ); + } }); diff --git a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/child.svelte b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/child.svelte new file mode 100644 index 000000000000..77a04f3e9f82 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/child.svelte @@ -0,0 +1,8 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte index ebbf42aa7c1b..0e7390d164f3 100644 --- a/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/store-from-props-hoisting/main.svelte @@ -1,10 +1,7 @@ - + \ No newline at end of file From f86ad91df6d66f7f5c7f53af66e88f807845436f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 29 Apr 2024 09:25:48 +0200 Subject: [PATCH 3/3] changeset --- .changeset/tiny-moose-kiss.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/tiny-moose-kiss.md b/.changeset/tiny-moose-kiss.md index e37fa126a391..1c4ec34c6037 100644 --- a/.changeset/tiny-moose-kiss.md +++ b/.changeset/tiny-moose-kiss.md @@ -2,4 +2,4 @@ "svelte": patch --- -fix: store from props hoist wrong param +fix: ensure store from props is hoisted correctly