diff --git a/.changeset/moody-sheep-type.md b/.changeset/moody-sheep-type.md new file mode 100644 index 0000000000..b218d70bad --- /dev/null +++ b/.changeset/moody-sheep-type.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: make `bind_this` implementation more robust 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 1b95031bb6..46906b3e98 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 @@ -35,6 +35,7 @@ import { import { regex_is_valid_identifier } from '../../../patterns.js'; import { javascript_visitors_runes } from './javascript-runes.js'; import { sanitize_template_string } from '../../../../utils/sanitize_template_string.js'; +import { walk } from 'zimmerframe'; /** * @param {import('#compiler').RegularElement | import('#compiler').SvelteElement} element @@ -978,24 +979,11 @@ function serialize_inline_component(node, component_name, context) { if (bind_this !== null) { const prev = fn; - const assignment = b.assignment('=', bind_this, b.id('$$value')); - const bind_this_id = /** @type {import('estree').Expression} */ ( - // if expression is not an identifier, we know it can't be a signal - bind_this.type === 'Identifier' - ? bind_this - : bind_this.type === 'MemberExpression' && bind_this.object.type === 'Identifier' - ? bind_this.object - : undefined - ); fn = (node_id) => - b.call( - '$.bind_this', - prev(node_id), - b.arrow( - [b.id('$$value')], - serialize_set_binding(assignment, context, () => context.visit(assignment)) - ), - bind_this_id + serialize_bind_this( + /** @type {import('estree').Identifier | import('estree').MemberExpression} */ (bind_this), + context, + prev(node_id) ); } @@ -1022,6 +1010,66 @@ function serialize_inline_component(node, component_name, context) { return statements.length > 1 ? b.block(statements) : statements[0]; } +/** + * Serializes `bind:this` for components and elements. + * @param {import('estree').Identifier | import('estree').MemberExpression} bind_this + * @param {import('zimmerframe').Context} context + * @param {import('estree').Expression} node + * @returns + */ +function serialize_bind_this(bind_this, context, node) { + let i = 0; + /** @type {Map} */ + const each_ids = new Map(); + // Transform each reference to an each block context variable into a $$value_ variable + // by temporarily changing the `expression` of the corresponding binding. + // These $$value_ variables will be filled in by the bind_this runtime function through its last argument. + // Note that we only do this for each context variables, the consequence is that the value might be stale in + // some scenarios where the value is a member expression with changing computed parts or using a combination of multiple + // variables, but that was the same case in Svelte 4, too. Once legacy mode is gone completely, we can revisit this. + walk( + bind_this, + {}, + { + Identifier(node) { + const binding = context.state.scope.get(node.name); + if (!binding || each_ids.has(binding)) return; + + const associated_node = Array.from(context.state.scopes.entries()).find( + ([_, scope]) => scope === binding?.scope + )?.[0]; + if (associated_node?.type === 'EachBlock') { + each_ids.set(binding, [ + i, + /** @type {import('estree').Expression} */ (context.visit(node)), + binding.expression + ]); + binding.expression = b.id('$$value_' + i); + i++; + } + } + } + ); + + const bind_this_id = /** @type {import('estree').Expression} */ (context.visit(bind_this)); + const ids = Array.from(each_ids.values()).map((id) => b.id('$$value_' + id[0])); + const assignment = b.assignment('=', bind_this, b.id('$$value')); + const update = serialize_set_binding(assignment, context, () => context.visit(assignment)); + + for (const [binding, [, , expression]] of each_ids) { + // reset expressions to what they were before + binding.expression = expression; + } + + /** @type {import('estree').Expression[]} */ + const args = [node, b.arrow([b.id('$$value'), ...ids], update), b.arrow([...ids], bind_this_id)]; + if (each_ids.size) { + args.push(b.thunk(b.array(Array.from(each_ids.values()).map((id) => id[1])))); + } + + return b.call('$.bind_this', ...args); +} + /** * Creates a new block which looks roughly like this: * ```js @@ -2749,19 +2797,7 @@ export const template_visitors = { } case 'this': - call_expr = b.call( - `$.bind_this`, - state.node, - setter, - /** @type {import('estree').Expression} */ ( - // if expression is not an identifier, we know it can't be a signal - expression.type === 'Identifier' - ? expression - : expression.type === 'MemberExpression' && expression.object.type === 'Identifier' - ? expression.object - : undefined - ) - ); + call_expr = serialize_bind_this(node.expression, context, state.node); break; case 'textContent': case 'innerHTML': diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index a6e5f80d5f..b3a048183b 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -36,7 +36,6 @@ import { } from './reconciler.js'; import { destroy_signal, - is_signal, push_destroy_fn, execute_effect, untrack, @@ -79,7 +78,6 @@ import { run } from '../common.js'; import { bind_transition, trigger_transitions } from './transitions.js'; import { mutable_source, source } from './reactivity/sources.js'; import { safe_equal, safe_not_equal } from './reactivity/equality.js'; -import { STATE_SYMBOL } from './constants.js'; /** @type {Set} */ const all_registerd_events = new Set(); @@ -1321,39 +1319,48 @@ export function bind_prop(props, prop, value) { } } -/** - * @param {unknown} value - */ -function is_state_object(value) { - return value != null && typeof value === 'object' && STATE_SYMBOL in value; -} - /** * @param {Element} element_or_component - * @param {(value: unknown) => void} update - * @param {import('./types.js').MaybeSignal} binding + * @param {(value: unknown, ...parts: unknown[]) => void} update + * @param {(...parts: unknown[]) => unknown} get_value + * @param {() => unknown[]} [get_parts] Set if the this binding is used inside an each block, + * returns all the parts of the each block context that are used in the expression * @returns {void} */ -export function bind_this(element_or_component, update, binding) { - render_effect(() => { - // If we are reading from a proxied state binding, then we don't need to untrack - // the update function as it will be fine-grain. - if (is_state_object(binding) || (is_signal(binding) && is_state_object(binding.v))) { - update(element_or_component); - } else { - untrack(() => update(element_or_component)); - } - return () => { - // Defer to the next tick so that all updates can be reconciled first. - // This solves the case where one variable is shared across multiple this-bindings. - render_effect(() => { - untrack(() => { - if (!is_signal(binding) || binding.v === element_or_component) { - update(null); - } - }); - }); - }; +export function bind_this(element_or_component, update, get_value, get_parts) { + /** @type {unknown[]} */ + let old_parts; + /** @type {unknown[]} */ + let parts; + + const e = effect(() => { + old_parts = parts; + // We only track changes to the parts, not the value itself to avoid unnecessary reruns. + parts = get_parts?.() || []; + + untrack(() => { + if (element_or_component !== get_value(...parts)) { + update(element_or_component, ...parts); + // If this is an effect rerun (cause: each block context changes), then nullfiy the binding at + // the previous position if it isn't already taken over by a different effect. + if (old_parts && get_value(...old_parts) === element_or_component) { + update(null, ...old_parts); + } + } + }); + }); + + // Add effect teardown (likely causes: if block became false, each item removed, component unmounted). + // In these cases we need to nullify the binding only if we detect that the value is still the same. + // If not, that means that another effect has now taken over the binding. + push_destroy_fn(e, () => { + // Defer to the next tick so that all updates can be reconciled first. + // This solves the case where one variable is shared across multiple this-bindings. + effect(() => { + if (get_value(...parts) === element_or_component) { + update(null, ...parts); + } + }); }); } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index e348d25f82..b53ebda54c 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -434,6 +434,7 @@ export function execute_effect(signal) { function infinite_loop_guard() { if (flush_count > 100) { + flush_count = 0; throw new Error( 'ERR_SVELTE_TOO_MANY_UPDATES' + (DEV diff --git a/packages/svelte/tests/runtime-legacy/samples/await-then-catch-event/_config.js b/packages/svelte/tests/runtime-legacy/samples/await-then-catch-event/_config.js index 33f5c7f4ab..26d22b8c1a 100644 --- a/packages/svelte/tests/runtime-legacy/samples/await-then-catch-event/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/await-then-catch-event/_config.js @@ -1,3 +1,4 @@ +import { tick } from 'svelte'; import { test } from '../../test'; import { create_deferred } from '../../../helpers.js'; @@ -22,7 +23,8 @@ export default test({ deferred.resolve(42); return deferred.promise - .then(() => { + .then(async () => { + await tick(); assert.htmlEqual(target.innerHTML, ''); const { button } = component; diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-this-with-context/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-this-with-context/_config.js index 6a4abf3edc..1688d15260 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-this-with-context/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-this-with-context/_config.js @@ -46,10 +46,8 @@ export default test({ component.items = ['foo', 'baz']; assert.equal(component.divs.length, 3, 'the divs array is still 3 long'); assert.equal(component.divs[2], null, 'the last div is unregistered'); - // Different from Svelte 3 - assert.equal(component.ps[1], null, 'the second p is unregistered'); - // Different from Svelte 3 - assert.equal(component.spans['-baz2'], null, 'the baz span is unregistered'); + assert.equal(component.ps[2], null, 'the last p is unregistered'); + assert.equal(component.spans['-bar1'], null, 'the bar span is unregistered'); assert.equal(component.hrs.bar, null, 'the bar hr is unregistered'); divs = target.querySelectorAll('div'); @@ -58,22 +56,17 @@ export default test({ assert.equal(e, divs[i], `div ${i} is still correct`); }); - // TODO: unsure if need these two tests to pass, as the logic between Svelte 3 - // and Svelte 5 is different for these cases. - - // elems = target.querySelectorAll('span'); - // component.items.forEach((e, i) => { - // assert.equal( - // component.spans[`-${e}${i}`], - // elems[i], - // `span -${e}${i} is still correct`, - // ); - // }); + spans = target.querySelectorAll('span'); + // @ts-ignore + component.items.forEach((e, i) => { + assert.equal(component.spans[`-${e}${i}`], spans[i], `span -${e}${i} is still correct`); + }); - // elems = target.querySelectorAll('p'); - // component.ps.forEach((e, i) => { - // assert.equal(e, elems[i], `p ${i} is still correct`); - // }); + ps = target.querySelectorAll('p'); + // @ts-ignore + component.ps.forEach((e, i) => { + assert.equal(e, ps[i], `p ${i} is still correct`); + }); hrs = target.querySelectorAll('hr'); // @ts-ignore diff --git a/packages/svelte/tests/runtime-legacy/samples/transition-js-if-block-outro-timeout/_config.js b/packages/svelte/tests/runtime-legacy/samples/transition-js-if-block-outro-timeout/_config.js index 6c4987cbf6..a110fbd042 100644 --- a/packages/svelte/tests/runtime-legacy/samples/transition-js-if-block-outro-timeout/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/transition-js-if-block-outro-timeout/_config.js @@ -1,3 +1,4 @@ +import { flushSync } from 'svelte'; import { test } from '../../test'; export default test({ @@ -15,7 +16,8 @@ export default test({ assert.equal(window.getComputedStyle(div).opacity, '0'); raf.tick(600); - assert.equal(component.div, undefined); assert.equal(target.querySelector('div'), undefined); + flushSync(); + assert.equal(component.div, undefined); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/_config.js b/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/_config.js new file mode 100644 index 0000000000..6d428f6306 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/_config.js @@ -0,0 +1,43 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +/** @param {number | null} selected */ +function get_html(selected) { + return ` + + + + +
+ + ${selected !== null ? `
${selected}
` : ''} + +
+ +

${selected ?? '...'}

+ `; +} + +export default test({ + html: get_html(null), + + async test({ assert, target }) { + const [btn1, btn2, btn3] = target.querySelectorAll('button'); + + await btn1?.click(); + await tick(); + assert.htmlEqual(target.innerHTML, get_html(1)); + + await btn2?.click(); + await tick(); + assert.htmlEqual(target.innerHTML, get_html(2)); + + await btn1?.click(); + await tick(); + assert.htmlEqual(target.innerHTML, get_html(1)); + + await btn3?.click(); + await tick(); + assert.htmlEqual(target.innerHTML, get_html(3)); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/main.svelte b/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/main.svelte new file mode 100644 index 0000000000..be400d1d9c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/bind-this-no-state/main.svelte @@ -0,0 +1,30 @@ + + +{#each [1, 2, 3] as n, i} + +{/each} + +
+ +{#each [1, 2, 3] as n, i} + {#if selected === i} +
{n}
+ {/if} +{/each} + +
+ +

{current ?? '...'}

\ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js index 0f261e1c81..78ae1ffc52 100644 --- a/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js @@ -11,7 +11,7 @@ export default function Bind_this($$anchor, $$props) { var fragment = $.comment($$anchor); var node = $.child_frag(fragment); - $.bind_this(Foo(node, {}), ($$value) => foo = $$value, foo); + $.bind_this(Foo(node, {}), ($$value) => foo = $$value, () => foo); $.close_frag($$anchor, fragment); $.pop(); } \ No newline at end of file