From d8f15195e3b7da47e788c2729ca72acd2b68938d Mon Sep 17 00:00:00 2001 From: rahim Date: Wed, 4 Feb 2026 20:27:44 +1100 Subject: [PATCH] refactor(store): replace signal/abort with signals namespace (#453) --- .../core/src/dom/store/features/source.ts | 4 +- packages/core/src/dom/store/features/time.ts | 49 ++--- packages/core/src/dom/store/signal-keys.ts | 3 + packages/store/README.md | 52 +++++- packages/store/src/core/index.ts | 1 + packages/store/src/core/selector.ts | 4 +- packages/store/src/core/signals.ts | 34 ++++ packages/store/src/core/slice.ts | 16 +- packages/store/src/core/store.ts | 27 +-- packages/store/src/core/tests/signals.test.ts | 173 ++++++++++++++++++ packages/store/src/core/tests/store.test.ts | 150 ++++++++------- 11 files changed, 383 insertions(+), 130 deletions(-) create mode 100644 packages/core/src/dom/store/signal-keys.ts create mode 100644 packages/store/src/core/signals.ts create mode 100644 packages/store/src/core/tests/signals.test.ts diff --git a/packages/core/src/dom/store/features/source.ts b/packages/core/src/dom/store/features/source.ts index 22de198a..1239868a 100644 --- a/packages/core/src/dom/store/features/source.ts +++ b/packages/core/src/dom/store/features/source.ts @@ -4,11 +4,11 @@ import type { SourceState } from '../../../core/media/state'; import { definePlayerFeature } from '../../feature'; export const sourceFeature = definePlayerFeature({ - state: ({ target, abort }): SourceState => ({ + state: ({ target, signals }): SourceState => ({ source: null, canPlay: false, loadSource(src: string) { - abort(); // Cancel pending operations (e.g., seek) + signals.clear(); // Cancel pending operations (e.g., seek) const { media } = target(); media.src = src; diff --git a/packages/core/src/dom/store/features/time.ts b/packages/core/src/dom/store/features/time.ts index 2d951ac2..306bc073 100644 --- a/packages/core/src/dom/store/features/time.ts +++ b/packages/core/src/dom/store/features/time.ts @@ -3,40 +3,31 @@ import { noop } from '@videojs/utils/function'; import type { TimeState } from '../../../core/media/state'; import { definePlayerFeature } from '../../feature'; import { hasMetadata } from '../../media/predicate'; +import { signalKeys } from '../signal-keys'; export const timeFeature = definePlayerFeature({ - state: ({ target, signal }): TimeState => { - let abort: AbortController | null = null; + state: ({ target, signals }): TimeState => ({ + currentTime: 0, + duration: 0, + seeking: false, + async seek(time: number) { + const { media } = target(), + signal = signals.supersede(signalKeys.seek); - const supersede = () => { - abort?.abort(); - abort = new AbortController(); - return AbortSignal.any([signal(), abort.signal]); - }; + // If metadata isn't loaded, wait for it before seeking to avoid errors. + if (!hasMetadata(media)) { + const loaded = await onEvent(media, 'loadedmetadata', { signal }).catch(() => false); + if (!loaded) return media.currentTime; + } - return { - currentTime: 0, - duration: 0, - seeking: false, - async seek(time: number) { - const { media } = target(), - signal = supersede(); + // Perform the seek and wait for it to complete. + const clampedTime = Math.max(0, Math.min(time, media.duration || Infinity)); + media.currentTime = clampedTime; + await onEvent(media, 'seeked', { signal }).catch(noop); - // If metadata isn't loaded, wait for it before seeking to avoid errors. - if (!hasMetadata(media)) { - const loaded = await onEvent(media, 'loadedmetadata', { signal }).catch(() => false); - if (!loaded) return media.currentTime; - } - - // Perform the seek and wait for it to complete. - const clampedTime = Math.max(0, Math.min(time, media.duration || Infinity)); - media.currentTime = clampedTime; - await onEvent(media, 'seeked', { signal }).catch(noop); - - return media.currentTime; - }, - }; - }, + return media.currentTime; + }, + }), attach({ target, signal, set }) { const { media } = target; diff --git a/packages/core/src/dom/store/signal-keys.ts b/packages/core/src/dom/store/signal-keys.ts new file mode 100644 index 00000000..e814bc63 --- /dev/null +++ b/packages/core/src/dom/store/signal-keys.ts @@ -0,0 +1,3 @@ +export const signalKeys = { + seek: Symbol.for('@videojs/seek'), +} as const; diff --git a/packages/store/README.md b/packages/store/README.md index 3a994afc..432afbba 100644 --- a/packages/store/README.md +++ b/packages/store/README.md @@ -227,15 +227,44 @@ const unsubscribe = store.subscribe(() => { Mutations are auto-batched—multiple changes in the same tick trigger only one notification. +## Cancellation Signals + +Use `signals` to manage cancellation for async operations. The store provides a `Signals` instance that tracks the attach lifecycle and supports keyed cancellation for superseding work. + +```ts +state: ({ target, signals }) => ({ + // Supersede pattern: new seek cancels previous seek + async seek(time: number) { + const signal = signals.supersede(signalKeys.seek); + // ... + }, + + // Cancel all pending operations (e.g., when loading new source) + loadSource(src: string) { + signals.clear(); + // ... + }, +}), +``` + +**API:** + +| Method | Description | +|--------|-------------| +| `signals.base` | Attach-scoped signal. Aborts on detach or reattach. | +| `signals.supersede(key)` | Returns signal that aborts when same key is superseded or base aborts. | +| `signals.clear()` | Aborts all keyed signals, leaving base intact. | + +Define shared keys for cross-slice coordination: + +```ts +export const signalKeys = { + seek: Symbol.for('@videojs/seek'), +} as const; +``` + ## Error Handling -All store errors include a `code` for programmatic handling: - -| Code | Description | -| ------------ | ---------------------------- | -| `DESTROYED` | Store destroyed | -| `NO_TARGET` | No target attached | - Handle errors locally via `try/catch`, or globally via `onError`: ```ts @@ -264,9 +293,14 @@ try { } ``` -## Advanced +All store errors include a `code` for programmatic handling: -### State Primitives +| Code | Description | +| ------------ | ---------------------------- | +| `DESTROYED` | Store destroyed | +| `NO_TARGET` | No target attached | + +## State Primitives The store uses explicit state containers internally. You can use these primitives directly: diff --git a/packages/store/src/core/index.ts b/packages/store/src/core/index.ts index f506ec56..0d7248a7 100644 --- a/packages/store/src/core/index.ts +++ b/packages/store/src/core/index.ts @@ -4,6 +4,7 @@ export * from './errors'; export { createSelector } from './selector'; export type { Comparator, Selector } from './shallow-equal'; export { shallowEqual } from './shallow-equal'; +export * from './signals'; export * from './slice'; export * from './state'; export * from './store'; diff --git a/packages/store/src/core/selector.ts b/packages/store/src/core/selector.ts index 95ec66db..0b4fd78d 100644 --- a/packages/store/src/core/selector.ts +++ b/packages/store/src/core/selector.ts @@ -1,11 +1,11 @@ import { pick } from '@videojs/utils/object'; import { throwNoTargetError } from './errors'; +import { Signals } from './signals'; import type { AnySlice, InferSliceState, StateContext } from './slice'; const stateContext: StateContext = { target: throwNoTargetError, - signal: throwNoTargetError, - abort: throwNoTargetError, + signals: new Signals(), }; /** diff --git a/packages/store/src/core/signals.ts b/packages/store/src/core/signals.ts new file mode 100644 index 00000000..01e14034 --- /dev/null +++ b/packages/store/src/core/signals.ts @@ -0,0 +1,34 @@ +export type SignalKey = PropertyKey; + +export class Signals { + #base = new AbortController(); + #keys = new Map(); + + /** The attach-scoped signal. Aborts on detach or reattach. */ + get base(): AbortSignal { + return this.#base.signal; + } + + /** Clears all keyed signals, leaving base intact. */ + clear(): void { + for (const controller of this.#keys.values()) { + controller.abort(); + } + this.#keys.clear(); + } + + /** Resets base and clears all keyed signals. */ + reset(): void { + this.clear(); + this.#base.abort(); + this.#base = new AbortController(); + } + + /** Creates a new signal for the key, superseding any previous signal. */ + supersede(key: SignalKey): AbortSignal { + this.#keys.get(key)?.abort(); + const controller = new AbortController(); + this.#keys.set(key, controller); + return AbortSignal.any([this.#base.signal, controller.signal]); + } +} diff --git a/packages/store/src/core/slice.ts b/packages/store/src/core/slice.ts index 807d8fb5..ec3ed7fa 100644 --- a/packages/store/src/core/slice.ts +++ b/packages/store/src/core/slice.ts @@ -1,4 +1,5 @@ import type { Simplify, UnionToIntersection } from '@videojs/utils/types'; +import type { Signals } from './signals'; import type { UnknownState } from './state'; // ---------------------------------------- @@ -28,10 +29,17 @@ export interface AttachContext { export interface StateContext { /** Returns the current target. Throws if not attached. */ target: () => Target; - /** Returns a signal that aborts on detach or when `abort()` is called. Throws if not attached. */ - signal: () => AbortSignal; - /** Aborts the current signal and creates a new one. Use to cancel pending operations. */ - abort: () => void; + /** + * Cancellation signals for async operations. + * + * - `signals.base` — Aborts on detach or reattach. Use for cleanup. + * - `signals.supersede(key)` — Returns a signal that aborts when the same key + * is superseded or when base aborts. Use for operations that should cancel + * previous in-flight work (e.g., seek superseding seek). + * - `signals.clear()` — Aborts all keyed signals. Use when starting fresh + * (e.g., loading a new source cancels pending seeks). + */ + signals: Signals; } // ---------------------------------------- diff --git a/packages/store/src/core/store.ts b/packages/store/src/core/store.ts index f7cbe52a..d841a818 100644 --- a/packages/store/src/core/store.ts +++ b/packages/store/src/core/store.ts @@ -1,6 +1,7 @@ import { isNull, isObject } from '@videojs/utils/predicate'; import type { StoreCallbacks } from './config'; import { throwDestroyedError, throwNoTargetError } from './errors'; +import { Signals } from './signals'; import type { AttachContext, Slice, StateContext } from './slice'; import type { StateChange, SubscribeOptions, UnknownState, WritableState } from './state'; import { createState } from './state'; @@ -19,10 +20,9 @@ export function createStore(): ( // Closure state let target: Target | null = null; let destroyed = false; - let attachAbort: AbortController | null = null; - let stateAbort = new AbortController(); const setupAbort = new AbortController(); + const signals = new Signals(); // Reactive state - initialized after building slice state let state: WritableState; @@ -37,14 +37,7 @@ export function createStore(): ( validate(); return target!; }, - signal: () => { - validate(); - return AbortSignal.any([attachAbort!.signal, stateAbort.signal]); - }, - abort: () => { - stateAbort.abort(); - stateAbort = new AbortController(); - }, + signals, } satisfies StateContext); state = createState(initialState); @@ -83,15 +76,14 @@ export function createStore(): ( function attach(newTarget: Target): () => void { if (destroyed) throwDestroyedError(); - attachAbort?.abort(); + // Reset signals for new attachment (also cleans up previous if reattaching) + signals.reset(); target = newTarget; - attachAbort = new AbortController(); - const signal = attachAbort.signal; // Create attach context const attachContext: AttachContext = { target: newTarget, - signal, + signal: signals.base, get: () => state.current, set: (partial) => state.patch(partial), reportError, @@ -113,7 +105,7 @@ export function createStore(): ( options.onAttach?.({ store, target: newTarget, - signal, + signal: signals.base, }); } catch (error) { reportError(error); @@ -124,10 +116,7 @@ export function createStore(): ( function detach(): void { if (isNull(target)) return; - stateAbort.abort(); - stateAbort = new AbortController(); - attachAbort?.abort(); - attachAbort = null; + signals.reset(); target = null; state.patch(initialState); } diff --git a/packages/store/src/core/tests/signals.test.ts b/packages/store/src/core/tests/signals.test.ts new file mode 100644 index 00000000..d704fc8f --- /dev/null +++ b/packages/store/src/core/tests/signals.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, it } from 'vitest'; +import { Signals } from '../signals'; + +describe('Signals', () => { + describe('base', () => { + it('returns an AbortSignal', () => { + const signals = new Signals(); + expect(signals.base).toBeInstanceOf(AbortSignal); + }); + + it('is not aborted initially', () => { + const signals = new Signals(); + expect(signals.base.aborted).toBe(false); + }); + + it('is aborted after reset()', () => { + const signals = new Signals(); + const base = signals.base; + + signals.reset(); + + expect(base.aborted).toBe(true); + }); + + it('returns new signal after reset()', () => { + const signals = new Signals(); + const base1 = signals.base; + + signals.reset(); + + const base2 = signals.base; + expect(base1).not.toBe(base2); + expect(base2.aborted).toBe(false); + }); + + it('is not aborted after clear()', () => { + const signals = new Signals(); + const base = signals.base; + + signals.clear(); + + expect(base.aborted).toBe(false); + }); + }); + + describe('clear', () => { + it('aborts keyed signals', () => { + const signals = new Signals(); + const signal = signals.supersede('test'); + + signals.clear(); + + expect(signal.aborted).toBe(true); + }); + + it('does not abort base', () => { + const signals = new Signals(); + const base = signals.base; + signals.supersede('test'); + + signals.clear(); + + expect(base.aborted).toBe(false); + }); + + it('clears all keyed signals', () => { + const signals = new Signals(); + const signal1 = signals.supersede('key1'); + const signal2 = signals.supersede('key2'); + + signals.clear(); + + expect(signal1.aborted).toBe(true); + expect(signal2.aborted).toBe(true); + }); + }); + + describe('reset', () => { + it('aborts base signal', () => { + const signals = new Signals(); + const base = signals.base; + + signals.reset(); + + expect(base.aborted).toBe(true); + }); + + it('aborts keyed signals', () => { + const signals = new Signals(); + const signal = signals.supersede('test'); + + signals.reset(); + + expect(signal.aborted).toBe(true); + }); + + it('creates new base signal', () => { + const signals = new Signals(); + const base1 = signals.base; + + signals.reset(); + + const base2 = signals.base; + expect(base1).not.toBe(base2); + expect(base2.aborted).toBe(false); + }); + }); + + describe('supersede', () => { + it('returns an AbortSignal', () => { + const signals = new Signals(); + const signal = signals.supersede('test'); + + expect(signal).toBeInstanceOf(AbortSignal); + }); + + it('is not aborted initially', () => { + const signals = new Signals(); + const signal = signals.supersede('test'); + + expect(signal.aborted).toBe(false); + }); + + it('aborts when base is reset', () => { + const signals = new Signals(); + const signal = signals.supersede('test'); + + signals.reset(); + + expect(signal.aborted).toBe(true); + }); + + it('aborts previous signal for same key', () => { + const signals = new Signals(); + const signal1 = signals.supersede('seek'); + + const signal2 = signals.supersede('seek'); + + expect(signal1.aborted).toBe(true); + expect(signal2.aborted).toBe(false); + }); + + it('does not abort signals with different keys', () => { + const signals = new Signals(); + const signal1 = signals.supersede('key1'); + const signal2 = signals.supersede('key2'); + + expect(signal1.aborted).toBe(false); + expect(signal2.aborted).toBe(false); + }); + + it('supports symbol keys', () => { + const signals = new Signals(); + const key = Symbol('test'); + const signal1 = signals.supersede(key); + const signal2 = signals.supersede(key); + + expect(signal1.aborted).toBe(true); + expect(signal2.aborted).toBe(false); + }); + + it('allows reusing key after clear()', () => { + const signals = new Signals(); + const signal1 = signals.supersede('test'); + + signals.clear(); + + const signal2 = signals.supersede('test'); + expect(signal1.aborted).toBe(true); + expect(signal2.aborted).toBe(false); + }); + }); +}); diff --git a/packages/store/src/core/tests/store.test.ts b/packages/store/src/core/tests/store.test.ts index 275aa5c8..2b96cc74 100644 --- a/packages/store/src/core/tests/store.test.ts +++ b/packages/store/src/core/tests/store.test.ts @@ -262,46 +262,34 @@ describe('store', () => { }); }); - describe('signal and abort', () => { - it('signal() throws when not attached', () => { + describe('signals', () => { + it('signals.base returns AbortSignal', () => { const slice = defineSlice()({ - state: ({ signal }) => ({ - getSignal: () => signal(), - }), - }); - - const store = createStore()(slice); - - expect(() => store.getSignal()).toThrow(); - }); - - it('signal() returns AbortSignal when attached', () => { - const slice = defineSlice()({ - state: ({ signal }) => ({ - getSignal: () => signal(), + state: ({ signals }) => ({ + getBase: () => signals.base, }), }); const store = createStore()(slice); store.attach(new MockMedia()); - const sig = store.getSignal(); + const sig = store.getBase(); expect(sig).toBeInstanceOf(AbortSignal); expect(sig.aborted).toBe(false); }); - it('signal aborts on detach', () => { + it('signals.base aborts on detach', () => { const slice = defineSlice()({ - state: ({ signal }) => ({ - getSignal: () => signal(), + state: ({ signals }) => ({ + getBase: () => signals.base, }), }); const store = createStore()(slice); const detach = store.attach(new MockMedia()); - const sig = store.getSignal(); + const sig = store.getBase(); expect(sig.aborted).toBe(false); detach(); @@ -309,62 +297,94 @@ describe('store', () => { expect(sig.aborted).toBe(true); }); - it('abort() aborts current signal', () => { + it('signals.base aborts on reattach', () => { const slice = defineSlice()({ - state: ({ signal, abort }) => ({ - getSignal: () => signal(), - abort: () => abort(), + state: ({ signals }) => ({ + getBase: () => signals.base, }), }); const store = createStore()(slice); store.attach(new MockMedia()); - const sig1 = store.getSignal(); - expect(sig1.aborted).toBe(false); - - store.abort(); - - expect(sig1.aborted).toBe(true); - }); - - it('abort() creates new signal for subsequent operations', () => { - const slice = defineSlice()({ - state: ({ signal, abort }) => ({ - getSignal: () => signal(), - abort: () => abort(), - }), - }); - - const store = createStore()(slice); - store.attach(new MockMedia()); - - const sig1 = store.getSignal(); - store.abort(); - - const sig2 = store.getSignal(); - - expect(sig1.aborted).toBe(true); - expect(sig2.aborted).toBe(false); - expect(sig1).not.toBe(sig2); - }); - - it('signal aborts on reattach', () => { - const slice = defineSlice()({ - state: ({ signal }) => ({ - getSignal: () => signal(), - }), - }); - - const store = createStore()(slice); - store.attach(new MockMedia()); - - const sig = store.getSignal(); + const sig = store.getBase(); expect(sig.aborted).toBe(false); store.attach(new MockMedia()); // Reattach expect(sig.aborted).toBe(true); }); + + it('signals.supersede() returns AbortSignal combined with base', () => { + const slice = defineSlice()({ + state: ({ signals }) => ({ + supersede: (key: string) => signals.supersede(key), + }), + }); + + const store = createStore()(slice); + store.attach(new MockMedia()); + + const sig = store.supersede('test'); + + expect(sig).toBeInstanceOf(AbortSignal); + expect(sig.aborted).toBe(false); + }); + + it('signals.supersede() aborts previous signal for same key', () => { + const slice = defineSlice()({ + state: ({ signals }) => ({ + supersede: (key: string) => signals.supersede(key), + }), + }); + + const store = createStore()(slice); + store.attach(new MockMedia()); + + const sig1 = store.supersede('seek'); + const sig2 = store.supersede('seek'); + + expect(sig1.aborted).toBe(true); + expect(sig2.aborted).toBe(false); + }); + + it('signals.supersede() aborts on detach', () => { + const slice = defineSlice()({ + state: ({ signals }) => ({ + supersede: (key: string) => signals.supersede(key), + }), + }); + + const store = createStore()(slice); + const detach = store.attach(new MockMedia()); + + const sig = store.supersede('test'); + expect(sig.aborted).toBe(false); + + detach(); + + expect(sig.aborted).toBe(true); + }); + + it('signals.clear() aborts keyed signals but not base', () => { + const slice = defineSlice()({ + state: ({ signals }) => ({ + getBase: () => signals.base, + supersede: (key: string) => signals.supersede(key), + clear: () => signals.clear(), + }), + }); + + const store = createStore()(slice); + store.attach(new MockMedia()); + + const base = store.getBase(); + const keyed = store.supersede('test'); + + store.clear(); + + expect(base.aborted).toBe(false); + expect(keyed.aborted).toBe(true); + }); }); });