diff --git a/biome.json b/biome.json index 5ba6939e..6ae4a78e 100644 --- a/biome.json +++ b/biome.json @@ -67,6 +67,10 @@ { "name": "useForceRender", "stableResult": true + }, + { + "name": "useLatestRef", + "stableResult": true } ] } diff --git a/packages/core/src/core/ui/buffering-indicator/buffering-indicator-core.ts b/packages/core/src/core/ui/buffering-indicator/buffering-indicator-core.ts index 3935eceb..2fd404f5 100644 --- a/packages/core/src/core/ui/buffering-indicator/buffering-indicator-core.ts +++ b/packages/core/src/core/ui/buffering-indicator/buffering-indicator-core.ts @@ -28,6 +28,10 @@ export class BufferingIndicatorCore { this.#props = defaults(props, BufferingIndicatorCore.defaultProps); } + destroy(): void { + this.#clearTimer(); + } + update(media: MediaPlaybackState): void { const buffering = media.waiting && !media.paused; @@ -37,14 +41,17 @@ export class BufferingIndicatorCore { this.state.patch({ visible: true }); }, this.#props.delay); } else if (!buffering) { - if (this.#timer !== null) { - clearTimeout(this.#timer); - this.#timer = null; - } - + this.#clearTimer(); this.state.patch({ visible: false }); } } + + #clearTimer(): void { + if (this.#timer !== null) { + clearTimeout(this.#timer); + this.#timer = null; + } + } } export namespace BufferingIndicatorCore { diff --git a/packages/react/src/media/dash-video/index.tsx b/packages/react/src/media/dash-video/index.tsx index 3baf7317..97401990 100644 --- a/packages/react/src/media/dash-video/index.tsx +++ b/packages/react/src/media/dash-video/index.tsx @@ -1,21 +1,15 @@ import { DashMedia } from '@videojs/core/dom/media/dash'; import type { PropsWithChildren, VideoHTMLAttributes } from 'react'; -import { forwardRef, useMemo } from 'react'; -import { useMediaAttach } from '../../player/context'; +import { forwardRef } from 'react'; import { attachMediaElement } from '../../utils/attach-media-element'; import { mediaProps } from '../../utils/media-props'; import { useComposedRefs } from '../../utils/use-composed-refs'; -import { useDestroy } from '../../utils/use-destroy'; +import { useMediaInstance } from '../../utils/use-media-instance'; export type DashVideoProps = PropsWithChildren>; export const DashVideo = forwardRef(({ children, ...props }, ref) => { - const mediaApi = useMemo(() => new DashMedia(), []); - const setMedia = useMediaAttach(); - - useDestroy(mediaApi, () => { - setMedia?.(mediaApi); - }); + const mediaApi = useMediaInstance(DashMedia); const composedRef = useComposedRefs(attachMediaElement(mediaApi), ref); diff --git a/packages/react/src/media/hls-video/index.tsx b/packages/react/src/media/hls-video/index.tsx index 5fd1e1cd..635a1e4d 100644 --- a/packages/react/src/media/hls-video/index.tsx +++ b/packages/react/src/media/hls-video/index.tsx @@ -1,21 +1,15 @@ import { HlsMedia } from '@videojs/core/dom/media/hls'; import type { PropsWithChildren, VideoHTMLAttributes } from 'react'; -import { forwardRef, useMemo } from 'react'; -import { useMediaAttach } from '../../player/context'; +import { forwardRef } from 'react'; import { attachMediaElement } from '../../utils/attach-media-element'; import { mediaProps } from '../../utils/media-props'; import { useComposedRefs } from '../../utils/use-composed-refs'; -import { useDestroy } from '../../utils/use-destroy'; +import { useMediaInstance } from '../../utils/use-media-instance'; export type HlsVideoProps = PropsWithChildren>; export const HlsVideo = forwardRef(({ children, ...props }, ref) => { - const mediaApi = useMemo(() => new HlsMedia(), []); - const setMedia = useMediaAttach(); - - useDestroy(mediaApi, () => { - setMedia?.(mediaApi); - }); + const mediaApi = useMediaInstance(HlsMedia); const composedRef = useComposedRefs(attachMediaElement(mediaApi), ref); diff --git a/packages/react/src/media/simple-hls-video/index.tsx b/packages/react/src/media/simple-hls-video/index.tsx index fb5e9e71..35567b9b 100644 --- a/packages/react/src/media/simple-hls-video/index.tsx +++ b/packages/react/src/media/simple-hls-video/index.tsx @@ -1,20 +1,15 @@ import { SimpleHlsMedia } from '@videojs/core/dom/media/simple-hls'; import type { PropsWithChildren, VideoHTMLAttributes } from 'react'; -import { forwardRef, useEffect, useMemo } from 'react'; -import { useMediaAttach } from '../../player/context'; +import { forwardRef } from 'react'; import { attachMediaElement } from '../../utils/attach-media-element'; import { mediaProps } from '../../utils/media-props'; import { useComposedRefs } from '../../utils/use-composed-refs'; +import { useMediaInstance } from '../../utils/use-media-instance'; export type SimpleHlsVideoProps = PropsWithChildren>; export const SimpleHlsVideo = forwardRef(({ children, ...props }, ref) => { - const mediaApi = useMemo(() => new SimpleHlsMedia(), []); - const setMedia = useMediaAttach(); - - useEffect(() => { - setMedia?.(mediaApi); - }, [mediaApi, setMedia]); + const mediaApi = useMediaInstance(SimpleHlsMedia); const composedRef = useComposedRefs(attachMediaElement(mediaApi), ref); diff --git a/packages/react/src/ui/buffering-indicator/buffering-indicator.tsx b/packages/react/src/ui/buffering-indicator/buffering-indicator.tsx index 6708c014..88f64977 100644 --- a/packages/react/src/ui/buffering-indicator/buffering-indicator.tsx +++ b/packages/react/src/ui/buffering-indicator/buffering-indicator.tsx @@ -7,6 +7,7 @@ import { forwardRef, useState, useSyncExternalStore } from 'react'; import { usePlayer } from '../../player/context'; import type { UIComponentProps } from '../../utils/types'; +import { useDestroy } from '../../utils/use-destroy'; import { renderElement } from '../../utils/use-render'; export interface BufferingIndicatorProps @@ -40,6 +41,7 @@ export const BufferingIndicator = forwardRef(function BufferingIndicator( const playback = usePlayer(selectPlayback); const [core] = useState(() => new BufferingIndicatorCore()); + useDestroy(core); core.setProps({ delay }); if (playback) core.update(playback); diff --git a/packages/react/src/ui/tooltip/tooltip-root.tsx b/packages/react/src/ui/tooltip/tooltip-root.tsx index 0b64d80a..1d0887b4 100644 --- a/packages/react/src/ui/tooltip/tooltip-root.tsx +++ b/packages/react/src/ui/tooltip/tooltip-root.tsx @@ -6,6 +6,7 @@ import { useSnapshot } from '@videojs/store/react'; import type { ReactNode } from 'react'; import { useEffect, useState } from 'react'; +import { useDestroy } from '../../utils/use-destroy'; import { useLatestRef } from '../../utils/use-latest-ref'; import { useSafeId } from '../../utils/use-safe-id'; import { TooltipContextProvider } from './context'; @@ -89,8 +90,7 @@ export function TooltipRoot({ } }, [controlledOpen, tooltip]); - // Cleanup on unmount - useEffect(() => () => tooltip.destroy(), [tooltip]); + useDestroy(tooltip); const input = useSnapshot(tooltip.input); core.setInput(input); diff --git a/packages/react/src/utils/tests/use-destroy.test.tsx b/packages/react/src/utils/tests/use-destroy.test.tsx index 937e3670..3f28b957 100644 --- a/packages/react/src/utils/tests/use-destroy.test.tsx +++ b/packages/react/src/utils/tests/use-destroy.test.tsx @@ -109,4 +109,40 @@ describe('useDestroy', () => { // Setup should only run once — the re-mount skips it expect(setup).toHaveBeenCalledOnce(); }); + + it('calls teardown before destroy on unmount', () => { + const order: string[] = []; + const instance = { destroy: vi.fn(() => order.push('destroy')) }; + const teardown = vi.fn(() => order.push('teardown')); + + const { unmount } = renderHook(() => useDestroy(instance, undefined, teardown)); + + unmount(); + vi.runAllTimers(); + + expect(teardown).toHaveBeenCalledOnce(); + expect(instance.destroy).toHaveBeenCalledOnce(); + expect(order).toEqual(['teardown', 'destroy']); + }); + + it('does not call teardown in StrictMode double-mount', () => { + const instance = { destroy: vi.fn() }; + const teardown = vi.fn(); + + function TestComponent() { + useDestroy(instance, undefined, teardown); + return null; + } + + render( + + + + ); + + vi.runAllTimers(); + + expect(teardown).not.toHaveBeenCalled(); + expect(instance.destroy).not.toHaveBeenCalled(); + }); }); diff --git a/packages/react/src/utils/use-destroy.ts b/packages/react/src/utils/use-destroy.ts index 4964d66e..2cf49b2a 100644 --- a/packages/react/src/utils/use-destroy.ts +++ b/packages/react/src/utils/use-destroy.ts @@ -2,6 +2,8 @@ import { useEffect, useRef } from 'react'; +import { useLatestRef } from './use-latest-ref'; + interface Destroyable { destroy(): void; } @@ -17,20 +19,27 @@ interface Destroyable { * @param instance - Object with a `destroy()` method. * @param setup - Optional setup called on first mount. Skipped on StrictMode * re-mount since the previous setup was never torn down. + * @param teardown - Optional teardown called right before `destroy()` on real + * unmount. Skipped on StrictMode simulated unmount. */ -export function useDestroy(instance: Destroyable, setup?: () => void): void { +export function useDestroy(instance: Destroyable, setup?: () => void, teardown?: () => void): void { const pendingRef = useRef | null>(null); + const setupRef = useLatestRef(setup); + const teardownRef = useLatestRef(teardown); useEffect(() => { if (pendingRef.current !== null) { clearTimeout(pendingRef.current); pendingRef.current = null; } else { - setup?.(); + setupRef.current?.(); } return () => { - pendingRef.current = setTimeout(() => instance.destroy(), 0); + pendingRef.current = setTimeout(() => { + teardownRef.current?.(); + instance.destroy(); + }, 0); }; - }, [instance, setup]); + }, [instance]); } diff --git a/packages/react/src/utils/use-media-instance.ts b/packages/react/src/utils/use-media-instance.ts new file mode 100644 index 00000000..481edf65 --- /dev/null +++ b/packages/react/src/utils/use-media-instance.ts @@ -0,0 +1,29 @@ +'use client'; + +import type { Media } from '@videojs/core/dom'; +import { useState } from 'react'; + +import { useMediaAttach } from '../player/context'; +import { useDestroy } from './use-destroy'; + +/** + * Create and manage a media instance lifecycle within a player context. + * + * Instantiates the media class once, attaches it to the player on mount, + * and safely detaches on unmount using a functional updater to avoid race + * conditions when swapping media components (e.g. DashVideo → HlsVideo). + */ +export function useMediaInstance( + MediaClass: new () => Instance +): Instance { + const [instance] = useState(() => new MediaClass()); + const setMedia = useMediaAttach(); + + useDestroy( + instance, + () => setMedia?.(instance), + () => setMedia?.((prev) => (prev === instance ? null : prev)) + ); + + return instance; +}