diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index 6f0097d05055d..6de4a261f3797 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,8 +11,8 @@ afterAll(() => { jest.clearAllMocks(); }); -describe(`${useDebouncedValue.name}`, () => { - function renderDebouncedValue(value: T, time: number) { +describe(useDebouncedValue.name, () => { + function renderDebouncedValue(value: T, time: number) { return renderHook( ({ value, time }: { value: T; time: number }) => { return useDebouncedValue(value, time); @@ -23,6 +23,25 @@ describe(`${useDebouncedValue.name}`, () => { ); } + it("Should throw for non-nonnegative integer timeouts", () => { + const invalidInputs: readonly number[] = [ + Number.NaN, + Number.NEGATIVE_INFINITY, + Number.POSITIVE_INFINITY, + Math.PI, + -42, + ]; + + const dummyValue = false; + for (const input of invalidInputs) { + expect(() => { + renderDebouncedValue(dummyValue, input); + }).toThrow( + `Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, + ); + } + }); + it("Should immediately return out the exact same value (by reference) on mount", () => { const value = {}; const { result } = renderDebouncedValue(value, 2000); @@ -58,6 +77,24 @@ describe(`${useDebouncedValue.name}`, () => { await jest.runAllTimersAsync(); await waitFor(() => expect(result.current).toEqual(true)); }); + + // Very important that we not do any async logic for this test + it("Should immediately resync without any render/event loop delays if timeout is zero", () => { + const initialValue = false; + const time = 5000; + + const { result, rerender } = renderDebouncedValue(initialValue, time); + expect(result.current).toEqual(false); + + // Just to be on the safe side, re-render once with the old timeout to + // verify that nothing has been flushed yet + rerender({ value: !initialValue, time }); + expect(result.current).toEqual(false); + + // Then do the real re-render once we know the coast is clear + rerender({ value: !initialValue, time: 0 }); + expect(result.current).toBe(true); + }); }); describe(`${useDebouncedFunction.name}`, () => { @@ -75,6 +112,27 @@ describe(`${useDebouncedFunction.name}`, () => { ); } + describe("input validation", () => { + it("Should throw for non-nonnegative integer timeouts", () => { + const invalidInputs: readonly number[] = [ + Number.NaN, + Number.NEGATIVE_INFINITY, + Number.POSITIVE_INFINITY, + Math.PI, + -42, + ]; + + const dummyFunction = jest.fn(); + for (const input of invalidInputs) { + expect(() => { + renderDebouncedFunction(dummyFunction, input); + }).toThrow( + `Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, + ); + } + }); + }); + describe("hook", () => { it("Should provide stable function references across re-renders", () => { const time = 5000; diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 945c927aad00c..0ed3d960d0ab2 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -2,18 +2,15 @@ * @file Defines hooks for created debounced versions of functions and arbitrary * values. * - * It is not safe to call a general-purpose debounce utility inside a React - * render. It will work on the initial render, but the memory reference for the - * value will change on re-renders. Most debounce functions create a "stateful" - * version of a function by leveraging closure; but by calling it repeatedly, - * you create multiple "pockets" of state, rather than a centralized one. - * - * Debounce utilities can make sense if they can be called directly outside the - * component or in a useEffect call, though. + * It is not safe to call most general-purpose debounce utility functions inside + * a React render. This is because the state for handling the debounce logic + * lives in the utility instead of React. If you call a general-purpose debounce + * function inline, that will create a new stateful function on every render, + * which has a lot of risks around conflicting/contradictory state. */ import { useCallback, useEffect, useRef, useState } from "react"; -type useDebouncedFunctionReturn = Readonly<{ +type UseDebouncedFunctionReturn = Readonly<{ debounced: (...args: Args) => void; // Mainly here to make interfacing with useEffect cleanup functions easier @@ -34,26 +31,32 @@ type useDebouncedFunctionReturn = Readonly<{ */ export function useDebouncedFunction< // Parameterizing on the args instead of the whole callback function type to - // avoid type contra-variance issues + // avoid type contravariance issues Args extends unknown[] = unknown[], >( callback: (...args: Args) => void | Promise, - debounceTimeMs: number, -): useDebouncedFunctionReturn { - const timeoutIdRef = useRef(null); + debounceTimeoutMs: number, +): UseDebouncedFunctionReturn { + if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { + throw new Error( + `Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, + ); + } + + const timeoutIdRef = useRef(undefined); const cancelDebounce = useCallback(() => { - if (timeoutIdRef.current !== null) { + if (timeoutIdRef.current !== undefined) { window.clearTimeout(timeoutIdRef.current); } - timeoutIdRef.current = null; + timeoutIdRef.current = undefined; }, []); - const debounceTimeRef = useRef(debounceTimeMs); + const debounceTimeRef = useRef(debounceTimeoutMs); useEffect(() => { cancelDebounce(); - debounceTimeRef.current = debounceTimeMs; - }, [cancelDebounce, debounceTimeMs]); + debounceTimeRef.current = debounceTimeoutMs; + }, [cancelDebounce, debounceTimeoutMs]); const callbackRef = useRef(callback); useEffect(() => { @@ -81,19 +84,32 @@ export function useDebouncedFunction< /** * Takes any value, and returns out a debounced version of it. */ -export function useDebouncedValue( - value: T, - debounceTimeMs: number, -): T { +export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { + if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { + throw new Error( + `Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, + ); + } + const [debouncedValue, setDebouncedValue] = useState(value); + // If the debounce timeout is ever zero, synchronously flush any state syncs. + // Doing this mid-render instead of in useEffect means that we drastically cut + // down on needless re-renders, and we also avoid going through the event loop + // to do a state sync that is *intended* to happen immediately + if (value !== debouncedValue && debounceTimeoutMs === 0) { + setDebouncedValue(value); + } useEffect(() => { + if (debounceTimeoutMs === 0) { + return; + } + const timeoutId = window.setTimeout(() => { setDebouncedValue(value); - }, debounceTimeMs); - + }, debounceTimeoutMs); return () => window.clearTimeout(timeoutId); - }, [value, debounceTimeMs]); + }, [value, debounceTimeoutMs]); return debouncedValue; }