fix(shared): give createPersistentStore a destroy() to dispose its effect.root
The store created an $effect.root for the save-on-change sync but returned no disposer, so the effect leaked for the life of the process — contradicting the rule that $effect.root owners must expose destroy(). Capture and expose the disposer. - add destroy() to the returned store; covered by tests (flushSync proves the save effect runs before destroy and stops after) - trim the bloated header (two near-duplicate @example blocks) to one concise JSDoc — no fluff - update typographySettings test mocks to satisfy the now-required destroy() Consumers (LayoutManager, ThemeManager, typographySettings, comparisonStore) do not yet call it — threading + the createSingleton migration follow.
This commit is contained in:
+3
@@ -51,6 +51,7 @@ describe('TypographySettingsStore - Unit Tests', () => {
|
||||
let mockPersistentStore: {
|
||||
value: TypographySettings;
|
||||
clear: () => void;
|
||||
destroy: () => void;
|
||||
};
|
||||
|
||||
const createMockPersistentStore = (initialValue: TypographySettings) => {
|
||||
@@ -70,6 +71,7 @@ describe('TypographySettingsStore - Unit Tests', () => {
|
||||
letterSpacing: DEFAULT_LETTER_SPACING,
|
||||
};
|
||||
},
|
||||
destroy() {},
|
||||
};
|
||||
};
|
||||
|
||||
@@ -535,6 +537,7 @@ describe('TypographySettingsStore - Unit Tests', () => {
|
||||
mockStorage = v;
|
||||
},
|
||||
clear: clearSpy,
|
||||
destroy() {},
|
||||
};
|
||||
|
||||
const manager = new TypographySettingsStore(
|
||||
|
||||
@@ -1,66 +1,15 @@
|
||||
/**
|
||||
* Persistent localStorage-backed reactive state
|
||||
* Reactive localStorage-backed state. Loads on init, saves on change via an
|
||||
* $effect.root. Falls back to the default on SSR (no localStorage) and on JSON
|
||||
* parse errors; swallows quota/write errors with a warning.
|
||||
*
|
||||
* Creates reactive state that automatically syncs with localStorage.
|
||||
* Values persist across browser sessions and are restored on page load.
|
||||
* Owners that create this outside a component must call destroy() to dispose
|
||||
* the save effect.
|
||||
*
|
||||
* Handles edge cases:
|
||||
* - SSR safety (no localStorage on server)
|
||||
* - JSON parse errors (falls back to default)
|
||||
* - Storage quota errors (logs warning, doesn't crash)
|
||||
*
|
||||
* @example
|
||||
* ```ts
|
||||
* // Store user preferences
|
||||
* const preferences = createPersistentStore('user-prefs', {
|
||||
* theme: 'dark',
|
||||
* fontSize: 16,
|
||||
* sidebarOpen: true
|
||||
* });
|
||||
*
|
||||
* // Access reactive state
|
||||
* $: currentTheme = preferences.value.theme;
|
||||
*
|
||||
* // Update (auto-saves to localStorage)
|
||||
* preferences.value.theme = 'light';
|
||||
*
|
||||
* // Clear stored value
|
||||
* preferences.clear();
|
||||
* ```
|
||||
*/
|
||||
|
||||
/**
|
||||
* Creates a reactive store backed by localStorage
|
||||
*
|
||||
* The value is loaded from localStorage on initialization and automatically
|
||||
* saved whenever it changes. Uses Svelte 5's $effect for reactive sync.
|
||||
*
|
||||
* @param key - localStorage key for storing the value
|
||||
* @param defaultValue - Default value if no stored value exists
|
||||
* @returns Persistent store with getter/setter and clear method
|
||||
*
|
||||
* @example
|
||||
* ```ts
|
||||
* // Simple value
|
||||
* const counter = createPersistentStore('counter', 0);
|
||||
* counter.value++;
|
||||
*
|
||||
* // Complex object
|
||||
* interface Settings {
|
||||
* theme: 'light' | 'dark';
|
||||
* fontSize: number;
|
||||
* }
|
||||
* const settings = createPersistentStore<Settings>('app-settings', {
|
||||
* theme: 'light',
|
||||
* fontSize: 16
|
||||
* });
|
||||
* ```
|
||||
* @param key - localStorage key
|
||||
* @param defaultValue - value used when nothing is stored
|
||||
*/
|
||||
export function createPersistentStore<T>(key: string, defaultValue: T) {
|
||||
/**
|
||||
* Load value from localStorage or return default
|
||||
* Safely handles missing keys, parse errors, and SSR
|
||||
*/
|
||||
const loadFromStorage = (): T => {
|
||||
if (typeof window === 'undefined') {
|
||||
return defaultValue;
|
||||
@@ -76,9 +25,13 @@ export function createPersistentStore<T>(key: string, defaultValue: T) {
|
||||
|
||||
let value = $state<T>(loadFromStorage());
|
||||
|
||||
// Sync to storage whenever value changes
|
||||
// Wrapped in $effect.root to prevent memory leaks
|
||||
$effect.root(() => {
|
||||
/**
|
||||
* Sync to storage whenever value changes. The effect lives in an
|
||||
* $effect.root so it outlives any component; the returned disposer is kept
|
||||
* and run by destroy(), because an $effect.root with no disposer leaks for
|
||||
* the life of the process.
|
||||
*/
|
||||
const dispose = $effect.root(() => {
|
||||
$effect(() => {
|
||||
if (typeof window === 'undefined') {
|
||||
return;
|
||||
@@ -113,6 +66,15 @@ export function createPersistentStore<T>(key: string, defaultValue: T) {
|
||||
}
|
||||
value = defaultValue;
|
||||
},
|
||||
|
||||
/**
|
||||
* Dispose the storage-sync effect. Owners that create a store outside a
|
||||
* component (e.g. a singleton store class) must call this to avoid
|
||||
* leaking the underlying $effect.root.
|
||||
*/
|
||||
destroy() {
|
||||
dispose();
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
/**
|
||||
* @vitest-environment jsdom
|
||||
*/
|
||||
import { flushSync } from 'svelte';
|
||||
import {
|
||||
afterEach,
|
||||
beforeEach,
|
||||
@@ -376,4 +377,39 @@ describe('createPersistentStore', () => {
|
||||
expect(store.value[0].name).toBe('First');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Lifecycle', () => {
|
||||
it('persists value changes via the sync effect', () => {
|
||||
const store = createPersistentStore(testKey, 'a');
|
||||
const spy = vi.spyOn(mockLocalStorage, 'setItem');
|
||||
|
||||
store.value = 'b';
|
||||
flushSync();
|
||||
|
||||
expect(spy).toHaveBeenCalledWith(testKey, JSON.stringify('b'));
|
||||
});
|
||||
|
||||
it('stops persisting after destroy()', () => {
|
||||
const store = createPersistentStore(testKey, 'a');
|
||||
flushSync();
|
||||
store.destroy();
|
||||
|
||||
const spy = vi.spyOn(mockLocalStorage, 'setItem');
|
||||
store.value = 'c';
|
||||
flushSync();
|
||||
|
||||
expect(spy).not.toHaveBeenCalled();
|
||||
// reading still works after disposal
|
||||
expect(store.value).toBe('c');
|
||||
});
|
||||
|
||||
it('destroy() is safe to call repeatedly', () => {
|
||||
const store = createPersistentStore(testKey, 'a');
|
||||
|
||||
expect(() => {
|
||||
store.destroy();
|
||||
store.destroy();
|
||||
}).not.toThrow();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user