Skip to content

Commit 41fb513

Browse files
fix: store access on component destroy (#14968)
Co-authored-by: Oscar Dominguez <[email protected]>
1 parent dbe5818 commit 41fb513

File tree

6 files changed

+96
-16
lines changed

6 files changed

+96
-16
lines changed

.changeset/hot-kings-shout.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
fix: store access on component destroy

packages/svelte/src/compiler/phases/3-transform/client/transform-client.js

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,8 @@ export function client_component(analysis, options) {
214214
/** @type {ESTree.VariableDeclaration[]} */
215215
const legacy_reactive_declarations = [];
216216

217+
let needs_store_cleanup = false;
218+
217219
for (const [name, binding] of analysis.instance.scope.declarations) {
218220
if (binding.kind === 'legacy_reactive') {
219221
legacy_reactive_declarations.push(
@@ -222,7 +224,10 @@ export function client_component(analysis, options) {
222224
}
223225
if (binding.kind === 'store_sub') {
224226
if (store_setup.length === 0) {
225-
store_setup.push(b.const('$$stores', b.call('$.setup_stores')));
227+
needs_store_cleanup = true;
228+
store_setup.push(
229+
b.const(b.array_pattern([b.id('$$stores'), b.id('$$cleanup')]), b.call('$.setup_stores'))
230+
);
226231
}
227232

228233
// 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) {
391396
analysis.reactive_statements.size > 0 ||
392397
component_returned_object.length > 0;
393398

399+
// we want the cleanup function for the stores to run as the very last thing
400+
// so that it can effectively clean up the store subscription even after the user effects runs
394401
if (should_inject_context) {
395402
component_block.body.unshift(b.stmt(b.call('$.push', ...push_args)));
396403

397-
component_block.body.push(
398-
component_returned_object.length > 0
399-
? b.return(b.call('$.pop', b.object(component_returned_object)))
400-
: b.stmt(b.call('$.pop'))
401-
);
404+
let to_push;
405+
406+
if (component_returned_object.length > 0) {
407+
let pop_call = b.call('$.pop', b.object(component_returned_object));
408+
to_push = needs_store_cleanup ? b.var('$$pop', pop_call) : b.return(pop_call);
409+
} else {
410+
to_push = b.stmt(b.call('$.pop'));
411+
}
412+
413+
component_block.body.push(to_push);
414+
}
415+
416+
if (needs_store_cleanup) {
417+
component_block.body.push(b.stmt(b.call('$$cleanup')));
418+
if (component_returned_object.length > 0) {
419+
component_block.body.push(b.return(b.id('$$pop')));
420+
}
402421
}
403422

404423
if (analysis.uses_rest_props) {

packages/svelte/src/internal/client/reactivity/store.js

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
/** @import { StoreReferencesContainer } from '#client' */
22
/** @import { Store } from '#shared' */
33
import { subscribe_to_store } from '../../../store/utils.js';
4-
import { noop } from '../../shared/utils.js';
4+
import { get as get_store } from '../../../store/shared/index.js';
5+
import { define_property, noop } from '../../shared/utils.js';
56
import { get } from '../runtime.js';
67
import { teardown } from './effects.js';
78
import { mutable_source, set } from './sources.js';
@@ -13,6 +14,8 @@ import { mutable_source, set } from './sources.js';
1314
*/
1415
let is_store_binding = false;
1516

17+
let IS_UNMOUNTED = Symbol();
18+
1619
/**
1720
* Gets the current value of a store. If the store isn't subscribed to yet, it will create a proxy
1821
* 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) {
3033
unsubscribe: noop
3134
});
3235

33-
if (entry.store !== store) {
36+
// if the component that setup this is already unmounted we don't want to register a subscription
37+
if (entry.store !== store && !(IS_UNMOUNTED in stores)) {
3438
entry.unsubscribe();
3539
entry.store = store ?? null;
3640

@@ -54,6 +58,13 @@ export function store_get(store, store_name, stores) {
5458
}
5559
}
5660

61+
// if the component that setup this stores is already unmounted the source will be out of sync
62+
// so we just use the `get` for the stores, less performant but it avoids to create a memory leak
63+
// and it will keep the value consistent
64+
if (store && IS_UNMOUNTED in stores) {
65+
return get_store(store);
66+
}
67+
5768
return get(entry.source);
5869
}
5970

@@ -103,20 +114,26 @@ export function invalidate_store(stores, store_name) {
103114

104115
/**
105116
* Unsubscribes from all auto-subscribed stores on destroy
106-
* @returns {StoreReferencesContainer}
117+
* @returns {[StoreReferencesContainer, ()=>void]}
107118
*/
108119
export function setup_stores() {
109120
/** @type {StoreReferencesContainer} */
110121
const stores = {};
111122

112-
teardown(() => {
113-
for (var store_name in stores) {
114-
const ref = stores[store_name];
115-
ref.unsubscribe();
116-
}
117-
});
123+
function cleanup() {
124+
teardown(() => {
125+
for (var store_name in stores) {
126+
const ref = stores[store_name];
127+
ref.unsubscribe();
128+
}
129+
define_property(stores, IS_UNMOUNTED, {
130+
enumerable: false,
131+
value: true
132+
});
133+
});
134+
}
118135

119-
return stores;
136+
return [stores, cleanup];
120137
}
121138

122139
/**
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<script lang="ts">
2+
const { store } = $props();
3+
4+
$effect(() => {
5+
$store;
6+
return () => {
7+
console.log($store);
8+
$store++;
9+
console.log($store);
10+
};
11+
});
12+
</script>
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { flushSync } from 'svelte';
2+
import { test } from '../../test';
3+
4+
export default test({
5+
async test({ assert, target, logs }) {
6+
const input = target.querySelector('input');
7+
flushSync(() => {
8+
input?.click();
9+
});
10+
assert.deepEqual(logs, [0, 1]);
11+
}
12+
});
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<script lang="ts">
2+
import { writable } from 'svelte/store';
3+
4+
import Test from './Test.svelte';
5+
6+
let store = writable(0);
7+
8+
let checked = $state(true);
9+
</script>
10+
11+
<input type="checkbox" bind:checked />
12+
13+
{#if checked}
14+
<Test {store} />
15+
{/if}

0 commit comments

Comments
 (0)