Skip to content

fix: eagerly unsubscribe when store is changed #10727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/short-countries-rush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: eagerly unsubscribe when store is changed
51 changes: 30 additions & 21 deletions packages/svelte/src/compiler/phases/3-transform/client/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -393,28 +393,37 @@ export function serialize_set_binding(node, context, fallback, options) {
return b.call(left, value);
} else if (is_store) {
return b.call('$.store_set', serialize_get_binding(b.id(left_name), state), value);
} else if (binding.kind === 'state') {
return b.call(
'$.set',
b.id(left_name),
context.state.analysis.runes &&
!options?.skip_proxy_and_freeze &&
should_proxy_or_freeze(value, context.state.scope)
? b.call('$.proxy', value)
: value
);
} else if (binding.kind === 'frozen_state') {
return b.call(
'$.set',
b.id(left_name),
context.state.analysis.runes &&
!options?.skip_proxy_and_freeze &&
should_proxy_or_freeze(value, context.state.scope)
? b.call('$.freeze', value)
: value
);
} else {
return b.call('$.set', b.id(left_name), value);
let call;
if (binding.kind === 'state') {
call = b.call(
'$.set',
b.id(left_name),
context.state.analysis.runes &&
!options?.skip_proxy_and_freeze &&
should_proxy_or_freeze(value, context.state.scope)
? b.call('$.proxy', value)
: value
);
} else if (binding.kind === 'frozen_state') {
call = b.call(
'$.set',
b.id(left_name),
context.state.analysis.runes &&
!options?.skip_proxy_and_freeze &&
should_proxy_or_freeze(value, context.state.scope)
? b.call('$.freeze', value)
: value
);
} else {
call = b.call('$.set', b.id(left_name), value);
}

if (state.scope.get(`$${left.name}`)?.kind === 'store_sub') {
return b.call('$.store_unsub', call, b.literal(`$${left.name}`), b.id('$$subscriptions'));
} else {
return call;
}
}
} else {
if (is_store) {
Expand Down
21 changes: 21 additions & 0 deletions packages/svelte/src/internal/client/reactivity/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,27 @@ export function store_get(store, store_name, stores) {
return value === UNINITIALIZED ? entry.last_value : value;
}

/**
* Unsubscribe from a store if it's not the same as the one in the store references container.
* We need this in addition to `store_get` because someone could unsubscribe from a store but
* then never subscribe to the new one (if any), causing the subscription to stay open wrongfully.
* @param {import('#client').Store<any> | null | undefined} store
* @param {string} store_name
* @param {import('#client').StoreReferencesContainer} stores
*/
export function store_unsub(store, store_name, stores) {
/** @type {import('#client').StoreReferencesContainer[''] | undefined} */
let entry = stores[store_name];

if (entry && entry.store !== store) {
// Don't reset store yet, so that store_get above can resubscribe to new store if necessary
entry.unsubscribe();
entry.unsubscribe = noop;
}

return store;
}

/**
* @template V
* @param {import('#client').Store<V> | null | undefined} store
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { tick } from 'svelte';
import { ok, test } from '../../test';

// Test that the store is unsubscribed from, even if it's not referenced once the store itself is set to null
export default test({
async test({ target, assert }) {
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>0</p> <button>add watcher</button>`
);

target.querySelector('button')?.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>1</p> 1 <button>remove watcher</button>`
);

const input = target.querySelector('input');
ok(input);

input.stepUp();
input.dispatchEvent(new Event('input', { bubbles: true }));
await tick();
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>2</p> 2 <button>remove watcher</button>`
);

target.querySelector('button')?.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>2</p> <button>add watcher</button>`
);

input.stepUp();
input.dispatchEvent(new Event('input', { bubbles: true }));
await tick();
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>2</p> <button>add watcher</button>`
);

target.querySelector('button')?.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<input type="number"> <p>3</p> 3 <button>remove watcher</button>`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<script>
import { writable, derived } from "svelte/store";

const obj = writable({ a: 1 });
let count = $state(0);
let watcherA = $state();

function watch (prop) {
return derived(obj, (o) => {
count++;
return o[prop];
});
}
</script>

<input type="number" bind:value={$obj.a}>
<p>{count}</p>

{#if watcherA}
{$watcherA}
<button on:click={() => watcherA = null}>remove watcher</button>
{:else}
<button on:click={() => watcherA = watch("a")}>add watcher</button>
{/if}