From 41fb51349e5846bdb406baba555d02cbf4f55b8e Mon Sep 17 00:00:00 2001 From: Paolo Ricciuti Date: Fri, 10 Jan 2025 11:28:18 +0100 Subject: [PATCH] fix: store access on component destroy (#14968) Co-authored-by: Oscar Dominguez --- .changeset/hot-kings-shout.md | 5 +++ .../3-transform/client/transform-client.js | 31 +++++++++++++--- .../src/internal/client/reactivity/store.js | 37 ++++++++++++++----- .../store-update-on-destroy/Test.svelte | 12 ++++++ .../store-update-on-destroy/_config.js | 12 ++++++ .../store-update-on-destroy/main.svelte | 15 ++++++++ 6 files changed, 96 insertions(+), 16 deletions(-) create mode 100644 .changeset/hot-kings-shout.md create mode 100644 packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/Test.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/main.svelte diff --git a/.changeset/hot-kings-shout.md b/.changeset/hot-kings-shout.md new file mode 100644 index 0000000000..afba164abf --- /dev/null +++ b/.changeset/hot-kings-shout.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: store access on component destroy 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 a969117ed3..582c32b534 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 @@ -214,6 +214,8 @@ export function client_component(analysis, options) { /** @type {ESTree.VariableDeclaration[]} */ const legacy_reactive_declarations = []; + let needs_store_cleanup = false; + for (const [name, binding] of analysis.instance.scope.declarations) { if (binding.kind === 'legacy_reactive') { legacy_reactive_declarations.push( @@ -222,7 +224,10 @@ export function client_component(analysis, options) { } if (binding.kind === 'store_sub') { if (store_setup.length === 0) { - store_setup.push(b.const('$$stores', b.call('$.setup_stores'))); + needs_store_cleanup = true; + store_setup.push( + b.const(b.array_pattern([b.id('$$stores'), b.id('$$cleanup')]), b.call('$.setup_stores')) + ); } // We're creating an arrow function that gets the store value which minifies better for two or more references @@ -391,14 +396,28 @@ export function client_component(analysis, options) { analysis.reactive_statements.size > 0 || component_returned_object.length > 0; + // we want the cleanup function for the stores to run as the very last thing + // so that it can effectively clean up the store subscription even after the user effects runs if (should_inject_context) { component_block.body.unshift(b.stmt(b.call('$.push', ...push_args))); - component_block.body.push( - component_returned_object.length > 0 - ? b.return(b.call('$.pop', b.object(component_returned_object))) - : b.stmt(b.call('$.pop')) - ); + let to_push; + + if (component_returned_object.length > 0) { + let pop_call = b.call('$.pop', b.object(component_returned_object)); + to_push = needs_store_cleanup ? b.var('$$pop', pop_call) : b.return(pop_call); + } else { + to_push = b.stmt(b.call('$.pop')); + } + + component_block.body.push(to_push); + } + + if (needs_store_cleanup) { + component_block.body.push(b.stmt(b.call('$$cleanup'))); + if (component_returned_object.length > 0) { + component_block.body.push(b.return(b.id('$$pop'))); + } } if (analysis.uses_rest_props) { diff --git a/packages/svelte/src/internal/client/reactivity/store.js b/packages/svelte/src/internal/client/reactivity/store.js index 11eee23e0a..e7a92ee052 100644 --- a/packages/svelte/src/internal/client/reactivity/store.js +++ b/packages/svelte/src/internal/client/reactivity/store.js @@ -1,7 +1,8 @@ /** @import { StoreReferencesContainer } from '#client' */ /** @import { Store } from '#shared' */ import { subscribe_to_store } from '../../../store/utils.js'; -import { noop } from '../../shared/utils.js'; +import { get as get_store } from '../../../store/shared/index.js'; +import { define_property, noop } from '../../shared/utils.js'; import { get } from '../runtime.js'; import { teardown } from './effects.js'; import { mutable_source, set } from './sources.js'; @@ -13,6 +14,8 @@ import { mutable_source, set } from './sources.js'; */ let is_store_binding = false; +let IS_UNMOUNTED = Symbol(); + /** * Gets the current value of a store. If the store isn't subscribed to yet, it will create a proxy * signal that will be updated when the store is. The store references container is needed to @@ -30,7 +33,8 @@ export function store_get(store, store_name, stores) { unsubscribe: noop }); - if (entry.store !== store) { + // if the component that setup this is already unmounted we don't want to register a subscription + if (entry.store !== store && !(IS_UNMOUNTED in stores)) { entry.unsubscribe(); entry.store = store ?? null; @@ -54,6 +58,13 @@ export function store_get(store, store_name, stores) { } } + // if the component that setup this stores is already unmounted the source will be out of sync + // so we just use the `get` for the stores, less performant but it avoids to create a memory leak + // and it will keep the value consistent + if (store && IS_UNMOUNTED in stores) { + return get_store(store); + } + return get(entry.source); } @@ -103,20 +114,26 @@ export function invalidate_store(stores, store_name) { /** * Unsubscribes from all auto-subscribed stores on destroy - * @returns {StoreReferencesContainer} + * @returns {[StoreReferencesContainer, ()=>void]} */ export function setup_stores() { /** @type {StoreReferencesContainer} */ const stores = {}; - teardown(() => { - for (var store_name in stores) { - const ref = stores[store_name]; - ref.unsubscribe(); - } - }); + function cleanup() { + teardown(() => { + for (var store_name in stores) { + const ref = stores[store_name]; + ref.unsubscribe(); + } + define_property(stores, IS_UNMOUNTED, { + enumerable: false, + value: true + }); + }); + } - return stores; + return [stores, cleanup]; } /** diff --git a/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/Test.svelte b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/Test.svelte new file mode 100644 index 0000000000..364a4a7aca --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/Test.svelte @@ -0,0 +1,12 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/_config.js b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/_config.js new file mode 100644 index 0000000000..bb99988756 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/_config.js @@ -0,0 +1,12 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + const input = target.querySelector('input'); + flushSync(() => { + input?.click(); + }); + assert.deepEqual(logs, [0, 1]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/main.svelte b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/main.svelte new file mode 100644 index 0000000000..7ba59b5afc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/main.svelte @@ -0,0 +1,15 @@ + + + + +{#if checked} + +{/if}