From a5331e00f90198e3341d0b72388556d4fa66fa2c Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:50:23 -0500 Subject: [PATCH 1/7] fix(useTimeout): make StrictMode safe --- src/useTimeout.ts | 66 ++++++++++++++++++++++++++++------------ test/useTimeout.test.tsx | 37 +++++++++++++++------- 2 files changed, 73 insertions(+), 30 deletions(-) diff --git a/src/useTimeout.ts b/src/useTimeout.ts index 1c61d72..d6c5edd 100644 --- a/src/useTimeout.ts +++ b/src/useTimeout.ts @@ -1,6 +1,5 @@ -import { MutableRefObject, useMemo, useRef } from 'react' +import { MutableRefObject, useEffect, useMemo, useRef, useState } from 'react' import useMounted from './useMounted' -import useWillUnmount from './useWillUnmount' /* * Browsers including Internet Explorer, Chrome, Safari, and Firefox store the @@ -28,12 +27,14 @@ function setChainedTimeout( ) } +type TimeoutState = { + fn: () => void + delayMs: number +} /** * Returns a controller object for setting a timeout that is properly cleaned up * once the component unmounts. New timeouts cancel and replace existing ones. * - * - * * ```tsx * const { set, clear } = useTimeout(); * const [hello, showHello] = useState(false); @@ -47,33 +48,60 @@ function setChainedTimeout( * ``` */ export default function useTimeout() { + const [timeout, setTimeoutState] = useState(null) const isMounted = useMounted() // types are confused between node and web here IDK - const handleRef = useRef() + const handleRef = useRef(null) - useWillUnmount(() => clearTimeout(handleRef.current)) + useEffect(() => { + if (!timeout) { + return + } - return useMemo(() => { - const clear = () => clearTimeout(handleRef.current) + const { fn, delayMs } = timeout - function set(fn: () => void, delayMs = 0): void { - if (!isMounted()) return + function task() { + if (isMounted()) { + setTimeoutState(null) + } + fn() + } - clear() + if (delayMs <= MAX_DELAY_MS) { + // For simplicity, if the timeout is short, just set a normal timeout. + handleRef.current = setTimeout(task, delayMs) + } else { + setChainedTimeout(handleRef, task, Date.now() + delayMs) + } + const handle = handleRef.current - if (delayMs <= MAX_DELAY_MS) { - // For simplicity, if the timeout is short, just set a normal timeout. - handleRef.current = setTimeout(fn, delayMs) - } else { - setChainedTimeout(handleRef, fn, Date.now() + delayMs) + return () => { + // this should be a no-op since they are either the same or `handle` + // already expired but no harm in calling twice + if (handleRef.current !== handle) { + clearTimeout(handle) } + + clearTimeout(handleRef.current) + handleRef.current === null } + }, [timeout]) + const [returnValue] = useState(() => { return { - set, - clear, + set(fn: () => void, delayMs = 0): void { + if (!isMounted()) return + + setTimeoutState({ fn, delayMs }) + }, + clear() { + setTimeoutState(null) + }, + isPending: !!timeout, handleRef, } - }, []) + }) + + return returnValue } diff --git a/test/useTimeout.test.tsx b/test/useTimeout.test.tsx index 6e86b71..68e8c88 100644 --- a/test/useTimeout.test.tsx +++ b/test/useTimeout.test.tsx @@ -1,6 +1,6 @@ import React from 'react' import useTimeout from '../src/useTimeout' -import { render } from '@testing-library/react' +import { render, act } from '@testing-library/react' describe('useTimeout', () => { it('should set a timeout', () => { @@ -17,11 +17,15 @@ describe('useTimeout', () => { render() - timeout!.set(spy, 100) + act(() => { + timeout!.set(spy, 100) + }) expect(spy).not.toHaveBeenCalled() - jest.runAllTimers() + act(() => { + jest.runAllTimers() + }) expect(spy).toHaveBeenCalledTimes(1) }) @@ -40,11 +44,14 @@ describe('useTimeout', () => { render() - timeout!.set(spy, 100) + act(() => { + timeout!.set(spy, 100) + }) - timeout!.clear() - - jest.runAllTimers() + act(() => { + timeout!.clear() + jest.runAllTimers() + }) expect(spy).toHaveBeenCalledTimes(0) }) @@ -63,11 +70,15 @@ describe('useTimeout', () => { const wrapper = render() - timeout!.set(spy, 100) + act(() => { + timeout!.set(spy, 100) + }) wrapper.unmount() - jest.runAllTimers() + act(() => { + jest.runAllTimers() + }) expect(spy).toHaveBeenCalledTimes(0) }) @@ -88,14 +99,18 @@ describe('useTimeout', () => { const MAX = 2 ** 31 - 1 - timeout!.set(spy, MAX + 100) + act(() => { + timeout!.set(spy, MAX + 100) + }) // some time to check that it didn't overflow and fire immediately jest.advanceTimersByTime(100) expect(spy).toHaveBeenCalledTimes(0) - jest.runAllTimers() + act(() => { + jest.runAllTimers() + }) expect(spy).toHaveBeenCalledTimes(1) }) From f0b8066f44f46304cd4a153b025db59c8a36388c Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:51:18 -0500 Subject: [PATCH 2/7] fix(useAnimationFrame)!: Make StrictMode safe BREAKING CHANGE: no longer supports `cancelPrevious` this is always true --- src/useAnimationFrame.ts | 61 ++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/src/useAnimationFrame.ts b/src/useAnimationFrame.ts index 47b0be9..9817132 100644 --- a/src/useAnimationFrame.ts +++ b/src/useAnimationFrame.ts @@ -1,7 +1,5 @@ -import { useRef } from 'react' +import { useEffect, useState } from 'react' import useMounted from './useMounted' -import useStableMemo from './useStableMemo' -import useWillUnmount from './useWillUnmount' export interface UseAnimationFrameReturn { cancel(): void @@ -11,15 +9,12 @@ export interface UseAnimationFrameReturn { * Previously registered callbacks will be cancelled */ request(callback: FrameRequestCallback): void - - /** - * Request for the provided callback to be called on the next animation frame. - * Previously registered callbacks can be cancelled by providing `cancelPrevious` - */ - request(cancelPrevious: boolean, callback: FrameRequestCallback): void +} +type AnimationFrameState = { + fn: FrameRequestCallback } /** - * Returns a controller object for requesting and cancelling an animation freame that is properly cleaned up + * Returns a controller object for requesting and cancelling an animation frame that is properly cleaned up * once the component unmounts. New requests cancel and replace existing ones. * * ```ts @@ -45,32 +40,32 @@ export interface UseAnimationFrameReturn { */ export default function useAnimationFrame(): UseAnimationFrameReturn { const isMounted = useMounted() - const handle = useRef() - const cancel = () => { - if (handle.current != null) { - cancelAnimationFrame(handle.current) - } - } + const [animationFrame, setAnimationFrameState] = + useState(null) - useWillUnmount(cancel) + useEffect(() => { + if (!animationFrame) { + return + } - return useStableMemo( - () => ({ - request( - cancelPrevious: boolean | FrameRequestCallback, - fn?: FrameRequestCallback, - ) { - if (!isMounted()) return + const { fn } = animationFrame + const handle = requestAnimationFrame(fn) + return () => { + cancelAnimationFrame(handle) + } + }, [animationFrame]) - if (cancelPrevious) cancel() + const [returnValue] = useState(() => ({ + request(callback: FrameRequestCallback) { + if (!isMounted()) return + setAnimationFrameState({ fn: callback }) + }, + cancel: () => { + if (!isMounted()) return + setAnimationFrameState(null) + }, + })) - handle.current = requestAnimationFrame( - fn || (cancelPrevious as FrameRequestCallback), - ) - }, - cancel, - }), - [], - ) + return returnValue } From ddeb67d1385d9cd4814eab1e65933f2659f9d1da Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:52:19 -0500 Subject: [PATCH 3/7] fix:(useDebouncedCallback): Clean up timeout logic in strict mode --- src/useDebouncedCallback.ts | 11 +++------- test/useDebouncedCallback.test.tsx | 33 ++++++++++++++++++++++-------- test/useDebouncedState.test.tsx | 12 ++++++----- 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/src/useDebouncedCallback.ts b/src/useDebouncedCallback.ts index e0bd617..e9f677e 100644 --- a/src/useDebouncedCallback.ts +++ b/src/useDebouncedCallback.ts @@ -1,7 +1,7 @@ import { useMemo, useRef } from 'react' import useTimeout from './useTimeout' import useEventCallback from './useEventCallback' -import useWillUnmount from './useWillUnmount' +import useMounted from './useMounted' export interface UseDebouncedCallbackOptions { wait: number @@ -71,11 +71,6 @@ function useDebouncedCallback any>( const timeout = useTimeout() - useWillUnmount(() => { - clearTimeout(timerRef.current) - isTimerSetRef.current = false - }) - return useMemo(() => { const hasMaxWait = !!maxWait @@ -168,14 +163,14 @@ function useDebouncedCallback any>( if (hasMaxWait) { // Handle invocations in a tight loop. isTimerSetRef.current = true - timerRef.current = setTimeout(timerExpired, wait) + timeout.set(timerExpired, wait) return invokeFunc(lastCallTimeRef.current) } } if (!isTimerSetRef.current) { isTimerSetRef.current = true - timerRef.current = setTimeout(timerExpired, wait) + timerRef.current = timeout.set(timerExpired, wait) } return returnValueRef.current diff --git a/test/useDebouncedCallback.test.tsx b/test/useDebouncedCallback.test.tsx index 259ba09..a83cd44 100644 --- a/test/useDebouncedCallback.test.tsx +++ b/test/useDebouncedCallback.test.tsx @@ -8,6 +8,12 @@ describe('useDebouncedCallback', () => { jest.useFakeTimers() }) + afterEach(() => { + act(() => { + jest.runAllTimers() + }) + }) + it('should return a function that debounces input callback', () => { const callback = jest.fn() @@ -21,7 +27,9 @@ describe('useDebouncedCallback', () => { expect(callback).not.toHaveBeenCalled() - jest.runOnlyPendingTimers() + act(() => { + jest.runOnlyPendingTimers() + }) expect(callback).toHaveBeenCalledTimes(1) expect(callback).toHaveBeenCalledWith(3) @@ -47,7 +55,6 @@ describe('useDebouncedCallback', () => { act(() => { jest.runOnlyPendingTimers() }) - expect(callback).toHaveBeenCalledTimes(1) }) @@ -90,16 +97,13 @@ describe('useDebouncedCallback', () => { result.current() result.current() result.current() - - setTimeout(() => { - result.current() - }, 1001) }) expect(callback).toHaveBeenCalledTimes(1) act(() => { jest.advanceTimersByTime(1001) + result.current() }) expect(callback).toHaveBeenCalledTimes(3) @@ -137,17 +141,25 @@ describe('useDebouncedCallback', () => { const callback = jest.fn(() => 42) const { result } = renderHook(() => useDebouncedCallback(callback, 1000)) + let retVal + + act(() => { + retVal = result.current() + }) - const retVal = result.current() expect(callback).toHaveBeenCalledTimes(0) expect(retVal).toBeUndefined() act(() => { jest.runAllTimers() }) + expect(callback).toHaveBeenCalledTimes(1) - const subsequentResult = result.current() + let subsequentResult + act(() => { + subsequentResult = result.current() + }) expect(callback).toHaveBeenCalledTimes(1) expect(subsequentResult).toBe(42) @@ -160,7 +172,10 @@ describe('useDebouncedCallback', () => { useDebouncedCallback(callback, { wait: 1000, leading: true }), ) - const retVal = result.current() + let retVal + act(() => { + retVal = result.current() + }) expect(callback).toHaveBeenCalledTimes(1) expect(retVal).toEqual(42) diff --git a/test/useDebouncedState.test.tsx b/test/useDebouncedState.test.tsx index bd82fc0..8e2251c 100644 --- a/test/useDebouncedState.test.tsx +++ b/test/useDebouncedState.test.tsx @@ -17,11 +17,13 @@ describe('useDebouncedState', () => { const wrapper = render() expect(wrapper.getByText('0')).toBeTruthy() - outerSetValue((cur: number) => cur + 1) - outerSetValue((cur: number) => cur + 1) - outerSetValue((cur: number) => cur + 1) - outerSetValue((cur: number) => cur + 1) - outerSetValue((cur: number) => cur + 1) + act(() => { + outerSetValue((cur: number) => cur + 1) + outerSetValue((cur: number) => cur + 1) + outerSetValue((cur: number) => cur + 1) + outerSetValue((cur: number) => cur + 1) + outerSetValue((cur: number) => cur + 1) + }) expect(wrapper.getByText('0')).toBeTruthy() From 54590c57a13a04d2ce0e8e3b972a035c23bcba29 Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:53:01 -0500 Subject: [PATCH 4/7] chore: deprecate useWillUnmount This hook is not possible in StrictMode, and can cause bugs --- src/useWillUnmount.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/useWillUnmount.ts b/src/useWillUnmount.ts index b3bbf56..18c7973 100644 --- a/src/useWillUnmount.ts +++ b/src/useWillUnmount.ts @@ -5,6 +5,7 @@ import { useEffect } from 'react' * Attach a callback that fires when a component unmounts * * @param fn Handler to run when the component unmounts + * @deprecated Use `useMounted` and normal effects, this is not StrictMode safe * @category effects */ export default function useWillUnmount(fn: () => void) { From 3744b63fee99224458bf9650aadd439810213399 Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:53:42 -0500 Subject: [PATCH 5/7] fix(useForceUpdate): ensure that chained calls produce an update --- src/useForceUpdate.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/useForceUpdate.ts b/src/useForceUpdate.ts index ab633df..298580d 100644 --- a/src/useForceUpdate.ts +++ b/src/useForceUpdate.ts @@ -19,6 +19,6 @@ import { useReducer } from 'react' export default function useForceUpdate(): () => void { // The toggling state value is designed to defeat React optimizations for skipping // updates when they are strictly equal to the last state value - const [, dispatch] = useReducer((state: boolean) => !state, false) + const [, dispatch] = useReducer((revision) => revision + 1, 0) return dispatch as () => void } From 6056f034deb8500cfe58208b68091c996b3e2b7d Mon Sep 17 00:00:00 2001 From: jquense Date: Fri, 22 Nov 2024 15:53:47 -0500 Subject: [PATCH 6/7] Update useCustomEffect.ts --- src/useCustomEffect.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/useCustomEffect.ts b/src/useCustomEffect.ts index b1cabcd..e06c0a2 100644 --- a/src/useCustomEffect.ts +++ b/src/useCustomEffect.ts @@ -5,7 +5,6 @@ import { useEffect, useDebugValue, } from 'react' -import useWillUnmount from './useWillUnmount' import useMounted from './useMounted' export type EffectHook = (effect: EffectCallback, deps?: DependencyList) => void From 590790a53707455016d2828b0e4a389c97827895 Mon Sep 17 00:00:00 2001 From: jquense Date: Sat, 23 Nov 2024 09:55:26 -0500 Subject: [PATCH 7/7] address feedback --- src/useDebouncedCallback.ts | 5 +---- src/useTimeout.ts | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/useDebouncedCallback.ts b/src/useDebouncedCallback.ts index e9f677e..6e91ea0 100644 --- a/src/useDebouncedCallback.ts +++ b/src/useDebouncedCallback.ts @@ -1,7 +1,6 @@ import { useMemo, useRef } from 'react' import useTimeout from './useTimeout' import useEventCallback from './useEventCallback' -import useMounted from './useMounted' export interface UseDebouncedCallbackOptions { wait: number @@ -55,8 +54,6 @@ function useDebouncedCallback any>( const isTimerSetRef = useRef(false) const lastArgsRef = useRef(null) - // Use any to bypass type issue with setTimeout. - const timerRef = useRef(0) const handleCallback = useEventCallback(fn) @@ -170,7 +167,7 @@ function useDebouncedCallback any>( if (!isTimerSetRef.current) { isTimerSetRef.current = true - timerRef.current = timeout.set(timerExpired, wait) + timeout.set(timerExpired, wait) } return returnValueRef.current diff --git a/src/useTimeout.ts b/src/useTimeout.ts index d6c5edd..5856985 100644 --- a/src/useTimeout.ts +++ b/src/useTimeout.ts @@ -1,4 +1,4 @@ -import { MutableRefObject, useEffect, useMemo, useRef, useState } from 'react' +import { MutableRefObject, useEffect, useRef, useState } from 'react' import useMounted from './useMounted' /*