From b32b80e55bd37b28f7704fdd9848e635ee318850 Mon Sep 17 00:00:00 2001 From: Wesley Luyten Date: Wed, 17 Jun 2026 17:58:52 -0700 Subject: [PATCH] refactor(core): rework media config as a plain getter/setter (#1697) --- .../src/dom/media/hls/tests/hls-media.test.ts | 14 +- packages/core/src/dom/media/media-host.ts | 28 ++-- .../src/dom/media/mux/tests/index.test.ts | 17 +- .../src/dom/media/tests/media-host.test.ts | 152 +++++++++++------- .../core/src/dom/media/tests/utils.test.ts | 23 +++ .../media/{utils.ts => utils/components.ts} | 48 +++--- packages/core/src/dom/media/utils/index.ts | 1 + .../html/src/media/tests/mux-video.test.ts | 20 ++- 8 files changed, 195 insertions(+), 108 deletions(-) create mode 100644 packages/core/src/dom/media/tests/utils.test.ts rename packages/core/src/dom/media/{utils.ts => utils/components.ts} (50%) create mode 100644 packages/core/src/dom/media/utils/index.ts diff --git a/packages/core/src/dom/media/hls/tests/hls-media.test.ts b/packages/core/src/dom/media/hls/tests/hls-media.test.ts index 41a056ad..2c7a2dcd 100644 --- a/packages/core/src/dom/media/hls/tests/hls-media.test.ts +++ b/packages/core/src/dom/media/hls/tests/hls-media.test.ts @@ -136,7 +136,7 @@ describe('HlsMedia', () => { media.addEventListener('streamtypechange', handler); // New `hlsJs` option values must recreate the engine to take effect. - media.config = { hlsJs: { maxBufferLength: 60 } }; + media.config = { ...media.config, hlsJs: { maxBufferLength: 60 } }; media.load(); // Teardown `live` → `unknown`, then the new delegate re-detects `live`. @@ -147,7 +147,7 @@ describe('HlsMedia', () => { it('does not recreate the engine for an equivalent hlsJs config', () => { const { media, video } = setup(); - media.config = { hlsJs: { maxBufferLength: 60 } }; + media.config = { ...media.config, hlsJs: { maxBufferLength: 60 } }; media.load(); fireDurationChange(video, Infinity); @@ -155,20 +155,22 @@ describe('HlsMedia', () => { media.addEventListener('streamtypechange', handler); // Same option values in a new object (e.g. an inline React prop). - media.config = { hlsJs: { maxBufferLength: 60 } }; + media.config = { ...media.config, hlsJs: { maxBufferLength: 60 } }; media.load(); // No engine teardown → no streamType churn. expect(handler).not.toHaveBeenCalled(); }); - it('merges config assignments', () => { + it('resets free-form config when a new object is assigned', () => { const { media } = setup(); media.config = { hlsJs: { maxBufferLength: 60 } }; - // Keys from the previous assignment in `setup()` survive. - expect(media.config.preferPlayback).toBe('native'); + // A new config object signals a fresh start: prior free-form keys + // (set in `setup()`) are dropped rather than merged. + expect(media.config.preferPlayback).toBeUndefined(); + expect(media.config.contentType).toBeUndefined(); expect(media.config.hlsJs).toEqual({ maxBufferLength: 60 }); }); }); diff --git a/packages/core/src/dom/media/media-host.ts b/packages/core/src/dom/media/media-host.ts index e7541b3e..709f61d0 100644 --- a/packages/core/src/dom/media/media-host.ts +++ b/packages/core/src/dom/media/media-host.ts @@ -9,7 +9,7 @@ import { type TextTrackLike, } from '../../core/media/types'; import { EMPTY_REMOTE, EMPTY_TEXT_TRACKS, EMPTY_TIME_RANGES } from './constants'; -import { getComponents, getProp, setProp } from './utils'; +import { getComponents, getOwner, getProp, setProp } from './utils'; export { addComponent, getComponents, getOwner, getProp, setProp } from './utils'; @@ -89,8 +89,6 @@ export class HTMLMediaElementHost { it('exposes mux config under host.config.muxData with inferred types', () => { const media = new HlsMedia(); - addComponent(media, new MuxData()); + const muxData = new MuxData(); + addComponent(media, muxData); // Type-level: `config.muxData` infers `Partial` via the - // component's `configKey` augmentation, so these assignments/reads are - // checked. This line fails to compile if inference regresses. - media.config.muxData = { envKey: 'key', debug: true }; + // component's `configKey` augmentation, so the assignment/read are checked. + // This fails to compile if inference regresses. + media.config = { muxData: { envKey: 'key', debug: true } }; const envKey: string | undefined = media.config.muxData?.envKey; expect(envKey).toBe('key'); - // Live binding: the write reached the component instance. - expect(media.config.muxData).toBeInstanceOf(MuxData); + // `config` stores the plain namespace POJO, not the component instance. + expect(media.config.muxData).toEqual({ envKey: 'key', debug: true }); + expect(media.config.muxData).not.toBeInstanceOf(MuxData); + // The setter still routed those values onto the live component instance. + expect(muxData.envKey).toBe('key'); + expect(muxData.debug).toBe(true); }); it('stops re-monitoring after destroy', async () => { diff --git a/packages/core/src/dom/media/tests/media-host.test.ts b/packages/core/src/dom/media/tests/media-host.test.ts index d5fa5f9f..f6f7ec11 100644 --- a/packages/core/src/dom/media/tests/media-host.test.ts +++ b/packages/core/src/dom/media/tests/media-host.test.ts @@ -191,85 +191,111 @@ describe('HTMLMediaElementHost', () => { }); describe('component config binding', () => { - it('exposes the component instance under its configKey', () => { + it('applies a component namespace onto the component when config is set', () => { const host = new HTMLAudioElementHost(); const component = new ConfigurableComponent(); addComponent(host, component); - expect(host.config.fake).toBe(component); - }); - - it('reads live values from the component', () => { - const host = new HTMLAudioElementHost(); - const component = new ConfigurableComponent(); - addComponent(host, component); - - component.value = 5; - expect((host.config.fake as ConfigurableComponent).value).toBe(5); - }); - - it('assigns onto the component when the namespace is written', () => { - const host = new HTMLAudioElementHost(); - const component = new ConfigurableComponent(); - addComponent(host, component); - - host.config.fake = { value: 7, label: 'hi' }; - - expect(component.value).toBe(7); - expect(component.label).toBe('hi'); - }); - - it('routes component keys through the config setter', () => { - const host = new HTMLAudioElementHost(); - const component = new ConfigurableComponent(); - addComponent(host, component); - - host.config = { fake: { value: 3 }, hlsJs: { debug: true } }; + host.config = { fake: { value: 3, label: 'a' } }; expect(component.value).toBe(3); - // Component instance is not stored as a plain object on the host. - expect(host.config.fake).toBe(component); - // Non-component keys are stored on the host bag. - expect(host.config.hlsJs).toEqual({ debug: true }); + expect(component.label).toBe('a'); }); - it('reflects a component added after the first config access', () => { - const host = new HTMLAudioElementHost(); - // Access config before the component exists to build the proxy. - expect(host.config.fake).toBeUndefined(); - - const component = new ConfigurableComponent(); - addComponent(host, component); - - expect(host.config.fake).toBe(component); - }); - - it('includes the configKey in has/ownKeys', () => { + it('stores config as plain values, never component instances', () => { const host = new HTMLAudioElementHost(); addComponent(host, new ConfigurableComponent()); - expect('fake' in host.config).toBe(true); - expect(Object.keys(host.config)).toContain('fake'); + host.config = { fake: { value: 3 }, hlsJs: { debug: true } }; + + // `config` is a plain bag of what was set — reading it back yields the + // assigned POJO, never the component instance. + expect(host.config.fake).toEqual({ value: 3 }); + expect(host.config.fake).not.toBeInstanceOf(ConfigurableComponent); + expect(host.config.hlsJs).toEqual({ debug: true }); }); - it('merges host-level keys on assignment', () => { + it('returns the same object that was assigned', () => { + const host = new HTMLAudioElementHost(); + const value = { fake: { value: 1 }, a: 2 }; + + host.config = value; + + expect(host.config).toBe(value); + }); + + it('round-trips through JSON without leaking component instances', () => { + const host = new HTMLAudioElementHost(); + addComponent(host, new ConfigurableComponent()); + + host.config = { fake: { value: 5, label: 'a' }, a: 1 }; + + // The stringified getter is valid input to the setter — plain values only. + const serialized = JSON.parse(JSON.stringify(host.config)); + expect(serialized).toEqual({ fake: { value: 5, label: 'a' }, a: 1 }); + }); + + it('does not apply config when the returned object is mutated directly', () => { + const host = new HTMLAudioElementHost(); + const component = new ConfigurableComponent(); + addComponent(host, component); + + // Only the setter applies namespaces to components; mutating the bag in + // place bypasses it. + host.config.fake = { value: 7, label: 'hi' }; + + expect(component.value).toBe(0); + expect(component.label).toBe(''); + }); + + it('replaces the entire config object on set', () => { const host = new HTMLAudioElementHost(); host.config = { a: 1 }; host.config = { b: 2 }; - expect(host.config.a).toBe(1); + // A new object replaces the old one wholesale; prior keys are dropped. + expect(host.config.a).toBeUndefined(); expect(host.config.b).toBe(2); }); - it('removes the config binding when the component is removed', () => { + it('keeps component state when a later config omits its namespace', () => { const host = new HTMLAudioElementHost(); - const remove = addComponent(host, new ConfigurableComponent()); + const component = new ConfigurableComponent(); + addComponent(host, component); + + host.config = { fake: { value: 5 }, a: 1 }; + host.config = { b: 2 }; + + // The component retains its applied state even though the new config + // object no longer lists its namespace. + expect(component.value).toBe(5); + expect(host.config.fake).toBeUndefined(); + expect(host.config.a).toBeUndefined(); + expect(host.config.b).toBe(2); + }); + + it('overwrites component state only for keys present in the new config', () => { + const host = new HTMLAudioElementHost(); + const component = new ConfigurableComponent(); + addComponent(host, component); + + host.config = { fake: { value: 5, label: 'a' } }; + host.config = { fake: { value: 9 } }; + + expect(component.value).toBe(9); + expect(component.label).toBe('a'); + }); + + it('stops applying config to a removed component', () => { + const host = new HTMLAudioElementHost(); + const component = new ConfigurableComponent(); + const remove = addComponent(host, component); remove(); + host.config = { fake: { value: 7 } }; - expect(host.config.fake).toBeUndefined(); - expect('fake' in host.config).toBe(false); + expect(component.value).toBe(0); }); it('adopts config set before the component was registered', () => { @@ -281,7 +307,23 @@ describe('HTMLMediaElementHost', () => { expect(component.value).toBe(4); expect(component.label).toBe('early'); - expect(host.config.fake).toBe(component); + // The plain value stays in the bag; it was never replaced. + expect(host.config.fake).toEqual({ value: 4, label: 'early' }); + }); + + it('drops pre-registration component config after an intervening config reset', () => { + const host = new HTMLAudioElementHost(); + host.config = { fake: { value: 4, label: 'early' } }; + // A later config object replaces the bag wholesale, so the staged value is + // gone before the component registers. + host.config = { a: 1 }; + + const component = new ConfigurableComponent(); + addComponent(host, component); + + expect(component.value).toBe(0); + expect(component.label).toBe(''); + expect(host.config.fake).toBeUndefined(); }); }); }); diff --git a/packages/core/src/dom/media/tests/utils.test.ts b/packages/core/src/dom/media/tests/utils.test.ts new file mode 100644 index 00000000..9d7fb7ca --- /dev/null +++ b/packages/core/src/dom/media/tests/utils.test.ts @@ -0,0 +1,23 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { HTMLAudioElementHost } from '../audio-host'; +import { getProp } from '../utils'; + +afterEach(() => { + document.body.innerHTML = ''; +}); + +describe('getProp', () => { + it('returns the owner value', () => { + const host = new HTMLAudioElementHost(); + const audio = document.createElement('audio'); + audio.loop = true; + host.attach(audio); + + expect(getProp(host, 'loop')).toBe(true); + }); + + it('returns undefined when nothing is attached', () => { + const host = new HTMLAudioElementHost(); + expect(getProp(host, 'loop')).toBeUndefined(); + }); +}); diff --git a/packages/core/src/dom/media/utils.ts b/packages/core/src/dom/media/utils/components.ts similarity index 50% rename from packages/core/src/dom/media/utils.ts rename to packages/core/src/dom/media/utils/components.ts index 874889e8..4bfa823e 100644 --- a/packages/core/src/dom/media/utils.ts +++ b/packages/core/src/dom/media/utils/components.ts @@ -1,56 +1,48 @@ -import { isFunction } from '@videojs/utils/predicate'; import type { Component, ComponentConstructor, Components, HTMLMediaElementHost, HTMLMediaTargetLike as TargetLike, -} from './media-host'; +} from '../media-host'; -type Host = HTMLMediaElementHost; +export type Host = HTMLMediaElementHost; -const registry = new WeakMap(); +const componentRegistry = new WeakMap(); export function getComponents(host: Host) { - let map = registry.get(host); - if (!map) registry.set(host, (map = new Map() as Components)); + let map = componentRegistry.get(host); + if (!map) componentRegistry.set(host, (map = new Map() as Components)); return map; } -export function addComponent(host: Host, instance: T) { +export function addComponent(host: Host, component: T) { const components = getComponents(host); - const ctor = instance.constructor as ComponentConstructor; - components.set(ctor, instance); + // Get the component's constructor to use as the key for the component in the registry. + const ctor = component.constructor as ComponentConstructor; - // Expose a live binding on `host.config`: reads return the component, writes assign onto it. + // Adopt any config set under this namespace before the component registered. const { configKey } = ctor; - if (configKey) { - // Adopt config set before the component was registered. - const initial = host.config[configKey]; - Object.defineProperty(host.config, configKey, { - enumerable: true, - configurable: true, - get: () => instance, - set: (value) => Object.assign(instance, value), - }); - if (initial) Object.assign(instance, initial); - } + const staged = configKey ? host.config[configKey] : undefined; + + components.set(ctor, component); + + if (staged !== undefined) Object.assign(component, staged); + + component.setMedia?.(host); - instance.setMedia?.(host); // @ts-expect-error `target` is protected, but these helpers are the host's own machinery. - if (host.target) instance.attach?.(host.target); + if (host.target) component.attach?.(host.target); + return () => { - if (components.get(ctor) === instance) { + if (components.get(ctor) === component) { components.delete(ctor); - if (configKey) delete host.config[configKey]; } }; } export function getProp(host: Host, prop: K): T[K] | undefined { - const own = getOwner(host, prop); - const result = own?.[prop]; - return isFunction(result) ? (result.bind(own) as T[K]) : result; + return getOwner(host, prop)?.[prop]; } export function setProp(host: Host, prop: K, value: T[K]): void { diff --git a/packages/core/src/dom/media/utils/index.ts b/packages/core/src/dom/media/utils/index.ts new file mode 100644 index 00000000..9bb4a2ca --- /dev/null +++ b/packages/core/src/dom/media/utils/index.ts @@ -0,0 +1 @@ +export { addComponent, getComponents, getOwner, getProp, setProp } from './components'; diff --git a/packages/html/src/media/tests/mux-video.test.ts b/packages/html/src/media/tests/mux-video.test.ts index b0d9bfa1..fe003b48 100644 --- a/packages/html/src/media/tests/mux-video.test.ts +++ b/packages/html/src/media/tests/mux-video.test.ts @@ -1,3 +1,4 @@ +import { getComponents } from '@videojs/core/dom/media/media-host'; import { MuxData } from '@videojs/core/dom/media/mux'; import { afterEach, describe, expect, it } from 'vitest'; import { MuxVideo } from '../mux-video'; @@ -17,18 +18,31 @@ afterEach(() => { }); describe('MuxVideo', () => { - it('exposes the mux data component config on element config', () => { + it('constructs the mux data component with the player software name', () => { + const el = createMuxVideo(); + const muxData = getComponents(el.host).get(MuxData); + + expect(muxData).toBeInstanceOf(MuxData); + expect(muxData?.playerSoftwareName).toBe('mux-video'); + }); + + it('exposes the element config as plain values, not the component instance', () => { const el = createMuxVideo(); - expect(el.config.muxData).toBeInstanceOf(MuxData); - expect((el.config.muxData as MuxData).playerSoftwareName).toBe('mux-video'); + // `config` reflects exactly what was set — a plain namespace bag. + expect(el.config.muxData).toEqual({ MuxDataSdk: undefined }); + expect(el.config.muxData).not.toBeInstanceOf(MuxData); }); it('routes component config writes to the component', () => { const el = createMuxVideo(); + const muxData = getComponents(el.host).get(MuxData); el.config = { muxData: { envKey: 'test-key' } }; + // The write reached the live component... + expect(muxData?.envKey).toBe('test-key'); + // ...and config reads back the plain value, not the instance. expect(el.config.muxData?.envKey).toBe('test-key'); expect(el.hasAttribute('config')).toBe(false); });