From 44d874ddc225b9b8aaf6d8689bdb48ef0ac729fb Mon Sep 17 00:00:00 2001 From: rahim Date: Thu, 5 Mar 2026 13:03:16 -0800 Subject: [PATCH] fix(react): strict mode support (#742) --- packages/react/src/player/create-player.tsx | 5 +- .../src/player/tests/create-player.test.tsx | 30 +++++ packages/react/src/ui/hooks/use-slider.ts | 6 +- .../react/src/ui/popover/popover-root.tsx | 4 +- packages/react/src/ui/thumbnail/thumbnail.tsx | 8 +- .../src/utils/tests/use-destroy.test.tsx | 112 ++++++++++++++++++ packages/react/src/utils/use-destroy.ts | 36 ++++++ 7 files changed, 189 insertions(+), 12 deletions(-) create mode 100644 packages/react/src/utils/tests/use-destroy.test.tsx create mode 100644 packages/react/src/utils/use-destroy.ts diff --git a/packages/react/src/player/create-player.tsx b/packages/react/src/player/create-player.tsx index 9b0aee41..e5c0e3a4 100644 --- a/packages/react/src/player/create-player.tsx +++ b/packages/react/src/player/create-player.tsx @@ -15,8 +15,9 @@ import type { InferStoreState } from '@videojs/store'; import { combine, createStore } from '@videojs/store'; import { useStore } from '@videojs/store/react'; import type { FC, ReactNode } from 'react'; -import { useEffect, useState } from 'react'; +import { useState } from 'react'; +import { useDestroy } from '../utils/use-destroy'; import { Container, PlayerContextProvider, useMedia, usePlayerContext } from './context'; export interface CreatePlayerConfig { @@ -71,7 +72,7 @@ export function createPlayer(config: CreatePlayerConfig): Cr const [store] = useState(() => createStore()(combine(...config.features))); const [media, setMedia] = useState(null); - useEffect(() => () => store.destroy(), [store]); + useDestroy(store); return {children}; } diff --git a/packages/react/src/player/tests/create-player.test.tsx b/packages/react/src/player/tests/create-player.test.tsx index cf03f1e3..54bfc319 100644 --- a/packages/react/src/player/tests/create-player.test.tsx +++ b/packages/react/src/player/tests/create-player.test.tsx @@ -2,6 +2,7 @@ import { render, renderHook } from '@testing-library/react'; import type { PlayerStore } from '@videojs/core/dom'; import { defineSlice } from '@videojs/store'; import type { ReactNode } from 'react'; +import { StrictMode } from 'react'; import { describe, expect, it, vi } from 'vitest'; import { createPlayer } from '../create-player'; @@ -39,6 +40,8 @@ describe('createPlayer', () => { }); it('destroys store on unmount', () => { + vi.useFakeTimers(); + const { Provider, usePlayer } = createPlayer({ features: [mockSlice] }); let store!: PlayerStore; @@ -56,8 +59,35 @@ describe('createPlayer', () => { const destroySpy = vi.spyOn(store, 'destroy'); unmount(); + vi.runAllTimers(); expect(destroySpy).toHaveBeenCalled(); + + vi.useRealTimers(); + }); + + it('survives React StrictMode without StoreError', () => { + const { Provider, usePlayer } = createPlayer({ features: [mockSlice] }); + + let store!: PlayerStore; + + function TestComponent() { + store = usePlayer(); + return null; + } + + expect(() => { + render( + + + + + + ); + }).not.toThrow(); + + expect(store).toBeDefined(); + expect(store.destroyed).toBe(false); }); it('uses displayName when provided', () => { diff --git a/packages/react/src/ui/hooks/use-slider.ts b/packages/react/src/ui/hooks/use-slider.ts index f96cd83d..12d102e9 100644 --- a/packages/react/src/ui/hooks/use-slider.ts +++ b/packages/react/src/ui/hooks/use-slider.ts @@ -10,7 +10,8 @@ import { } from '@videojs/core/dom'; import { useSnapshot } from '@videojs/store/react'; import { isRTL } from '@videojs/utils/dom'; -import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react'; +import { useCallback, useLayoutEffect, useRef, useState } from 'react'; +import { useDestroy } from '../../utils/use-destroy'; import { useForceRender } from '../../utils/use-force-render'; import { useLatestRef } from '../../utils/use-latest-ref'; @@ -82,8 +83,7 @@ export function useSlider( return createSlider(stableOptions); }); - // Cleanup on unmount. - useEffect(() => () => slider.destroy(), [slider]); + useDestroy(slider); // Subscribe to slider input state. const input = useSnapshot(slider.input); diff --git a/packages/react/src/ui/popover/popover-root.tsx b/packages/react/src/ui/popover/popover-root.tsx index 4063eda7..38f8740e 100644 --- a/packages/react/src/ui/popover/popover-root.tsx +++ b/packages/react/src/ui/popover/popover-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 { PopoverContextProvider } from './context'; @@ -85,8 +86,7 @@ export function PopoverRoot({ } }, [controlledOpen, popover]); - // Cleanup on unmount - useEffect(() => () => popover.destroy(), [popover]); + useDestroy(popover); const input = useSnapshot(popover.input); core.setInput(input); diff --git a/packages/react/src/ui/thumbnail/thumbnail.tsx b/packages/react/src/ui/thumbnail/thumbnail.tsx index be8b3c92..983967a5 100644 --- a/packages/react/src/ui/thumbnail/thumbnail.tsx +++ b/packages/react/src/ui/thumbnail/thumbnail.tsx @@ -9,10 +9,11 @@ import { } from '@videojs/core'; import { createThumbnail, selectTextTrack } from '@videojs/core/dom'; import type { CSSProperties } from 'react'; -import { forwardRef, useEffect, useMemo, useRef, useState } from 'react'; +import { forwardRef, useMemo, useRef, useState } from 'react'; import { useOptionalPlayer } from '../../player/context'; import type { UIComponentProps } from '../../utils/types'; +import { useDestroy } from '../../utils/use-destroy'; import { renderElement } from '../../utils/use-render'; export interface ThumbnailProps extends UIComponentProps<'div', ThumbnailCore.State>, ThumbnailCore.Props { @@ -49,10 +50,7 @@ export const Thumbnail = forwardRef(function Thu }) ); - useEffect(() => { - handle.connect(); - return () => handle.destroy(); - }, [handle]); + useDestroy(handle, () => handle.connect()); // Resolve thumbnails: external prop takes priority over auto path. const thumbnails = useMemo(() => { diff --git a/packages/react/src/utils/tests/use-destroy.test.tsx b/packages/react/src/utils/tests/use-destroy.test.tsx new file mode 100644 index 00000000..937e3670 --- /dev/null +++ b/packages/react/src/utils/tests/use-destroy.test.tsx @@ -0,0 +1,112 @@ +import { render, renderHook } from '@testing-library/react'; +import { StrictMode } from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { useDestroy } from '../use-destroy'; + +describe('useDestroy', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('destroys instance after unmount', () => { + const instance = { destroy: vi.fn() }; + + const { unmount } = renderHook(() => useDestroy(instance)); + + expect(instance.destroy).not.toHaveBeenCalled(); + + unmount(); + vi.runAllTimers(); + + expect(instance.destroy).toHaveBeenCalledOnce(); + }); + + it('does not destroy synchronously in cleanup', () => { + const instance = { destroy: vi.fn() }; + + const { unmount } = renderHook(() => useDestroy(instance)); + + unmount(); + + // Destroy is deferred — not called yet + expect(instance.destroy).not.toHaveBeenCalled(); + + vi.runAllTimers(); + expect(instance.destroy).toHaveBeenCalledOnce(); + }); + + it('does not destroy in StrictMode double-mount', () => { + const instance = { destroy: vi.fn() }; + + function TestComponent() { + useDestroy(instance); + return null; + } + + render( + + + + ); + + vi.runAllTimers(); + + // StrictMode runs cleanup then re-mount — destroy should be cancelled + expect(instance.destroy).not.toHaveBeenCalled(); + }); + + it('destroys after real unmount in StrictMode', () => { + const instance = { destroy: vi.fn() }; + + function TestComponent() { + useDestroy(instance); + return null; + } + + const { unmount } = render( + + + + ); + + vi.runAllTimers(); + expect(instance.destroy).not.toHaveBeenCalled(); + + unmount(); + vi.runAllTimers(); + + expect(instance.destroy).toHaveBeenCalledOnce(); + }); + + it('calls setup once on mount', () => { + const instance = { destroy: vi.fn() }; + const setup = vi.fn(); + + renderHook(() => useDestroy(instance, setup)); + + expect(setup).toHaveBeenCalledOnce(); + }); + + it('calls setup once in StrictMode (skips re-mount)', () => { + const instance = { destroy: vi.fn() }; + const setup = vi.fn(); + + function TestComponent() { + useDestroy(instance, setup); + return null; + } + + render( + + + + ); + + // Setup should only run once — the re-mount skips it + expect(setup).toHaveBeenCalledOnce(); + }); +}); diff --git a/packages/react/src/utils/use-destroy.ts b/packages/react/src/utils/use-destroy.ts new file mode 100644 index 00000000..4964d66e --- /dev/null +++ b/packages/react/src/utils/use-destroy.ts @@ -0,0 +1,36 @@ +'use client'; + +import { useEffect, useRef } from 'react'; + +interface Destroyable { + destroy(): void; +} + +/** + * Destroy an instance on unmount, deferring destruction to survive React + * StrictMode's simulated unmount/re-mount cycle. + * + * StrictMode runs effects, then immediately runs cleanup, then re-runs + * effects — all synchronously. By deferring `destroy()` to a macrotask, + * the re-mount effect can cancel it before it fires. + * + * @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. + */ +export function useDestroy(instance: Destroyable, setup?: () => void): void { + const pendingRef = useRef | null>(null); + + useEffect(() => { + if (pendingRef.current !== null) { + clearTimeout(pendingRef.current); + pendingRef.current = null; + } else { + setup?.(); + } + + return () => { + pendingRef.current = setTimeout(() => instance.destroy(), 0); + }; + }, [instance, setup]); +}