From b75316d23ffcf3e8c455dc3d05bc30d965f52409 Mon Sep 17 00:00:00 2001 From: rahim Date: Wed, 4 Feb 2026 15:32:59 +1100 Subject: [PATCH] refactor(packages): replace disposer with abort controller (#449) --- CLAUDE.md | 20 +- packages/html/src/store/container-mixin.ts | 11 +- packages/store/README.md | 18 ++ packages/store/src/core/state.ts | 26 ++- packages/store/src/core/store.ts | 8 +- packages/store/src/core/tests/state.test.ts | 13 ++ packages/store/src/core/tests/store.test.ts | 17 ++ packages/utils/src/events/disposer.ts | 48 ---- packages/utils/src/events/index.ts | 1 - .../utils/src/events/tests/disposer.test.ts | 212 ------------------ 10 files changed, 93 insertions(+), 281 deletions(-) delete mode 100644 packages/utils/src/events/disposer.ts delete mode 100644 packages/utils/src/events/tests/disposer.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index c513d032..45c82209 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -141,8 +141,8 @@ describe('SnapshotController', () => { ... }); // provider-mixin.test.ts — factory function export describe('createStoreProviderMixin', () => { ... }); -// disposer.test.ts — lowercase module/export -describe('disposer', () => { ... }); +// event-like.test.ts — lowercase module/export +describe('event-like', () => { ... }); ``` ## Guidelines @@ -337,20 +337,22 @@ destroy(): void { ### Cleanup Pattern -Use `Disposer` from `@videojs/utils/events` when managing multiple cleanup functions: +Use `AbortController` when managing multiple cleanups. It works with `listen` and any API that accepts a `signal`: ```ts -import { Disposer } from '@videojs/utils/events'; - -#disposer = new Disposer(); +#disconnect: AbortController | null = null; connect(): void { - this.#disposer.add(store.subscribe(...)); - this.#disposer.add(listen(element, 'click', handler)); + this.#disconnect?.abort(); + this.#disconnect = new AbortController(); + + store.subscribe(() => {}, { signal: this.#disconnect.signal }); + listen(element, 'click', handler, { signal: this.#disconnect.signal }); } disconnect(): void { - this.#disposer.dispose(); + this.#disconnect?.abort(); + this.#disconnect = null; } ``` diff --git a/packages/html/src/store/container-mixin.ts b/packages/html/src/store/container-mixin.ts index d28336d5..8c4c2c35 100644 --- a/packages/html/src/store/container-mixin.ts +++ b/packages/html/src/store/container-mixin.ts @@ -1,7 +1,6 @@ import { ContextConsumer } from '@lit/context'; import type { MediaContainer, PlayerStore, PlayerTarget } from '@videojs/core/dom'; import { listen, querySlot } from '@videojs/utils/dom'; -import { Disposer } from '@videojs/utils/events'; import { noop } from '@videojs/utils/function'; import type { MediaElementConstructor } from '@/ui/media-element'; import type { PlayerContext } from '../player/context'; @@ -15,7 +14,7 @@ export function createContainerMixin(context: PlayerC return (BaseClass: Class) => { class PlayerContainerElement extends BaseClass implements PlayerConsumer, MediaContainer { #detach = noop; - #disposer = new Disposer(); + #disconnect: AbortController | null = null; #consumer = new ContextConsumer(this, { context, @@ -30,9 +29,12 @@ export function createContainerMixin(context: PlayerC override connectedCallback() { super.connectedCallback(); + this.#disconnect?.abort(); + this.#disconnect = new AbortController(); + if (this.shadowRoot) { const slot = querySlot(this.shadowRoot, ''); - if (slot) this.#disposer.add(listen(slot, 'slotchange', () => this.#attachMedia())); + if (slot) listen(slot, 'slotchange', () => this.#attachMedia(), { signal: this.#disconnect.signal }); } this.#attachMedia(); @@ -40,7 +42,8 @@ export function createContainerMixin(context: PlayerC override disconnectedCallback() { super.disconnectedCallback(); - this.#disposer.dispose(); + this.#disconnect?.abort(); + this.#disconnect = null; this.#detach(); } diff --git a/packages/store/README.md b/packages/store/README.md index c4d7a44a..4aead5de 100644 --- a/packages/store/README.md +++ b/packages/store/README.md @@ -292,6 +292,19 @@ const unsubscribe = store.subscribe(() => { Mutations are auto-batched—multiple changes in the same tick trigger only one notification. +You can also pass an abort signal to clean up automatically: + +```ts +const controller = new AbortController(); + +store.subscribe(() => { + const { volume } = store; + console.log('State changed:', volume); +}, { signal: controller.signal }); + +controller.abort(); // unsubscribes +``` + ### Pending Tasks Track in-flight async operations: @@ -491,6 +504,11 @@ state.subscribe(() => { console.log('Changed:', volume); }); +// Optional abort signal for cleanup +const controller = new AbortController(); +state.subscribe(() => {}, { signal: controller.signal }); +controller.abort(); + // Check if value is state isState(state); // true diff --git a/packages/store/src/core/state.ts b/packages/store/src/core/state.ts index 37143c55..17b2da8b 100644 --- a/packages/store/src/core/state.ts +++ b/packages/store/src/core/state.ts @@ -1,10 +1,16 @@ +import { noop } from '@videojs/utils/function'; + export type StateChange = () => void; export type UnknownState = Record; +export interface SubscribeOptions { + signal?: AbortSignal; +} + export interface State { readonly current: Readonly; - subscribe(callback: StateChange): () => void; + subscribe(callback: StateChange, options?: SubscribeOptions): () => void; } export interface WritableState extends State { @@ -63,9 +69,23 @@ class StateContainer implements WritableState { } } - subscribe(callback: StateChange): () => void { + subscribe(callback: StateChange, options?: SubscribeOptions): () => void { + const signal = options?.signal; + if (signal?.aborted) return noop; + this.#listeners.add(callback); - return () => this.#listeners.delete(callback); + + if (!signal) { + return () => this.#listeners.delete(callback); + } + + const onAbort = () => this.#listeners.delete(callback); + signal.addEventListener('abort', onAbort, { once: true }); + + return () => { + signal.removeEventListener('abort', onAbort); + this.#listeners.delete(callback); + }; } flush(): void { diff --git a/packages/store/src/core/store.ts b/packages/store/src/core/store.ts index c2bd6a01..f7cbe52a 100644 --- a/packages/store/src/core/store.ts +++ b/packages/store/src/core/store.ts @@ -2,7 +2,7 @@ import { isNull, isObject } from '@videojs/utils/predicate'; import type { StoreCallbacks } from './config'; import { throwDestroyedError, throwNoTargetError } from './errors'; import type { AttachContext, Slice, StateContext } from './slice'; -import type { StateChange, UnknownState, WritableState } from './state'; +import type { StateChange, SubscribeOptions, UnknownState, WritableState } from './state'; import { createState } from './state'; const STORE_SYMBOL = Symbol('@videojs/store'); @@ -139,8 +139,8 @@ export function createStore(): ( setupAbort.abort(); } - function subscribe(callback: StateChange): () => void { - return state.subscribe(callback); + function subscribe(callback: StateChange, options?: SubscribeOptions): () => void { + return state.subscribe(callback, options); } function reportError(error: unknown): void { @@ -168,7 +168,7 @@ export interface BaseStore { readonly state: State; attach(target: Target): () => void; destroy(): void; - subscribe(callback: StateChange): () => void; + subscribe(callback: StateChange, options?: SubscribeOptions): () => void; } export type Store = BaseStore & State; diff --git a/packages/store/src/core/tests/state.test.ts b/packages/store/src/core/tests/state.test.ts index 60652c6c..44801809 100644 --- a/packages/store/src/core/tests/state.test.ts +++ b/packages/store/src/core/tests/state.test.ts @@ -103,6 +103,19 @@ describe('createState', () => { flush(); expect(listener).toHaveBeenCalledOnce(); // still 1 }); + + it('respects abort signal', () => { + const state = createTestState(); + const listener = vi.fn(); + const controller = new AbortController(); + + state.subscribe(listener, { signal: controller.signal }); + controller.abort(); + + state.patch({ volume: 0.5 }); + flush(); + expect(listener).not.toHaveBeenCalled(); + }); }); describe('isState', () => { diff --git a/packages/store/src/core/tests/store.test.ts b/packages/store/src/core/tests/store.test.ts index daf8b092..275aa5c8 100644 --- a/packages/store/src/core/tests/store.test.ts +++ b/packages/store/src/core/tests/store.test.ts @@ -203,6 +203,23 @@ describe('store', () => { expect(listener).not.toHaveBeenCalled(); }); + + it('respects abort signal', () => { + const store = createStore()(audioSlice); + + const media = new MockMedia(); + store.attach(media); + + const listener = vi.fn(); + const controller = new AbortController(); + store.subscribe(listener, { signal: controller.signal }); + + controller.abort(); + store.setVolume(0.5); + flush(); + + expect(listener).not.toHaveBeenCalled(); + }); }); describe('destroy', () => { diff --git a/packages/utils/src/events/disposer.ts b/packages/utils/src/events/disposer.ts deleted file mode 100644 index 3f47ecea..00000000 --- a/packages/utils/src/events/disposer.ts +++ /dev/null @@ -1,48 +0,0 @@ -/** - * A cleanup function that may be sync or async. - */ -export type CleanupFn = () => void | Promise; - -/** - * A collector for cleanup functions. - * - * Allows registering multiple cleanup functions and disposing them all at once. - * - * @example - * ```ts - * const disposer = new Disposer(); - * - * disposer.add(listen(video, 'play', handlePlay)); - * disposer.add(listen(video, 'pause', handlePause)); - * disposer.add(animationFrame(render)); - * - * // Later, clean up everything at once - * disposer.dispose(); - * // or for async cleanups: - * await disposer.disposeAsync(); - * ``` - */ -export class Disposer { - #cleanups = new Set(); - - get size(): number { - return this.#cleanups.size; - } - - add(cleanup: CleanupFn): void { - this.#cleanups.add(cleanup); - } - - /** Run all cleanups sync. Use `disposeAsync()` for async cleanups. */ - dispose(): void { - for (const cleanup of this.#cleanups) { - cleanup(); - } - this.#cleanups.clear(); - } - - async disposeAsync(): Promise { - await Promise.all([...this.#cleanups].map((cleanup) => cleanup())); - this.#cleanups.clear(); - } -} diff --git a/packages/utils/src/events/index.ts b/packages/utils/src/events/index.ts index af7ebb6d..6aa19b37 100644 --- a/packages/utils/src/events/index.ts +++ b/packages/utils/src/events/index.ts @@ -1,3 +1,2 @@ export * from './abort'; -export * from './disposer'; export * from './event-like'; diff --git a/packages/utils/src/events/tests/disposer.test.ts b/packages/utils/src/events/tests/disposer.test.ts deleted file mode 100644 index 26831778..00000000 --- a/packages/utils/src/events/tests/disposer.test.ts +++ /dev/null @@ -1,212 +0,0 @@ -import { describe, expect, it, vi } from 'vitest'; - -import { Disposer } from '../disposer'; - -describe('disposer', () => { - describe('constructor', () => { - it('creates a disposer with size 0', () => { - const disposer = new Disposer(); - expect(disposer.size).toBe(0); - }); - - it('tracks size as cleanups are added', () => { - const disposer = new Disposer(); - disposer.add(() => {}); - expect(disposer.size).toBe(1); - disposer.add(() => {}); - expect(disposer.size).toBe(2); - }); - }); - - describe('add', () => { - it('adds cleanup functions', () => { - const disposer = new Disposer(); - const cleanup1 = vi.fn(); - const cleanup2 = vi.fn(); - - disposer.add(cleanup1); - disposer.add(cleanup2); - - expect(disposer.size).toBe(2); - }); - - it('does not call cleanups when adding', () => { - const disposer = new Disposer(); - const cleanup = vi.fn(); - - disposer.add(cleanup); - - expect(cleanup).not.toHaveBeenCalled(); - }); - }); - - describe('dispose', () => { - it('calls all cleanup functions', () => { - const disposer = new Disposer(); - const cleanup1 = vi.fn(); - const cleanup2 = vi.fn(); - const cleanup3 = vi.fn(); - - disposer.add(cleanup1); - disposer.add(cleanup2); - disposer.add(cleanup3); - - disposer.dispose(); - - expect(cleanup1).toHaveBeenCalledOnce(); - expect(cleanup2).toHaveBeenCalledOnce(); - expect(cleanup3).toHaveBeenCalledOnce(); - }); - - it('clears the disposer after dispose', () => { - const disposer = new Disposer(); - disposer.add(() => {}); - disposer.add(() => {}); - - expect(disposer.size).toBe(2); - disposer.dispose(); - expect(disposer.size).toBe(0); - }); - - it('can be called multiple times safely', () => { - const disposer = new Disposer(); - const cleanup = vi.fn(); - - disposer.add(cleanup); - disposer.dispose(); - disposer.dispose(); - - expect(cleanup).toHaveBeenCalledOnce(); - }); - - it('allows adding new cleanups after dispose', () => { - const disposer = new Disposer(); - const cleanup1 = vi.fn(); - const cleanup2 = vi.fn(); - - disposer.add(cleanup1); - disposer.dispose(); - - disposer.add(cleanup2); - disposer.dispose(); - - expect(cleanup1).toHaveBeenCalledOnce(); - expect(cleanup2).toHaveBeenCalledOnce(); - }); - }); - - describe('disposeAsync', () => { - it('calls all cleanup functions', async () => { - const disposer = new Disposer(); - const cleanup1 = vi.fn(); - const cleanup2 = vi.fn(); - - disposer.add(cleanup1); - disposer.add(cleanup2); - - await disposer.disposeAsync(); - - expect(cleanup1).toHaveBeenCalledOnce(); - expect(cleanup2).toHaveBeenCalledOnce(); - }); - - it('awaits async cleanup functions', async () => { - const disposer = new Disposer(); - const order: string[] = []; - - disposer.add(async () => { - await Promise.resolve(); - order.push('async1'); - }); - - disposer.add(() => { - order.push('sync'); - }); - - disposer.add(async () => { - await Promise.resolve(); - order.push('async2'); - }); - - await disposer.disposeAsync(); - - expect(order).toContain('async1'); - expect(order).toContain('sync'); - expect(order).toContain('async2'); - }); - - it('clears the disposer after disposeAsync', async () => { - const disposer = new Disposer(); - disposer.add(async () => {}); - - expect(disposer.size).toBe(1); - await disposer.disposeAsync(); - expect(disposer.size).toBe(0); - }); - - it('handles mixed sync and async cleanups', async () => { - const disposer = new Disposer(); - const results: number[] = []; - - disposer.add(() => { - results.push(1); - }); - disposer.add(async () => { - await new Promise((r) => setTimeout(r, 10)); - results.push(2); - }); - disposer.add(() => { - results.push(3); - }); - - await disposer.disposeAsync(); - - expect(results).toHaveLength(3); - expect(results).toContain(1); - expect(results).toContain(2); - expect(results).toContain(3); - }); - }); - - describe('integration', () => { - it('works with real-world cleanup patterns', () => { - const disposer = new Disposer(); - - // Simulating event listener cleanup - const listeners = new Map void>(); - const addEventListener = (type: string, handler: () => void) => { - listeners.set(type, handler); - return () => { - listeners.delete(type); - }; - }; - - disposer.add(addEventListener('click', () => {})); - disposer.add(addEventListener('keydown', () => {})); - - expect(listeners.size).toBe(2); - disposer.dispose(); - expect(listeners.size).toBe(0); - }); - - it('works with timer cleanup patterns', () => { - vi.useFakeTimers(); - - const disposer = new Disposer(); - let timerFired = false; - - const id = setTimeout(() => { - timerFired = true; - }, 1000); - - disposer.add(() => clearTimeout(id)); - - disposer.dispose(); - vi.advanceTimersByTime(2000); - - expect(timerFired).toBe(false); - - vi.useRealTimers(); - }); - }); -});