fix: store access on component destroy (#14968)

Co-authored-by: Oscar Dominguez <[email protected]>

Author: paoloricciuti

.changeset/hot-kings-shout.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: store access on component destroy

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) {

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];
 }
 
 /**

packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/Test.svelte

@@ -0,0 +1,12 @@
+<script lang="ts">
+	const { store } = $props();
+
+	$effect(() => {
+		$store;
+		return () => {
+			console.log($store);
+			$store++;
+			console.log($store);
+		};
+	});
+</script>

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]);
+	}
+});

packages/svelte/tests/runtime-runes/samples/store-update-on-destroy/main.svelte

@@ -0,0 +1,15 @@
+<script lang="ts">
+	import { writable } from 'svelte/store';
+
+	import Test from './Test.svelte';
+
+	let store = writable(0);
+
+	let checked = $state(true);
+</script>
+
+<input type="checkbox" bind:checked />
+
+{#if checked}
+	<Test {store} />
+{/if}