From 32ebe810fd5f05998ea15edba2d06a6d9c24e79e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 13 Sep 2023 21:28:43 +0000 Subject: [PATCH 1/9] chore: Reorganize hook calls for useWorkspacesFilter --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 65b4262788f5d..f5f8013963cbc 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -130,7 +130,6 @@ const useWorkspacesFilter = ({ searchParamsResult, pagination, }: UseWorkspacesFilterOptions) => { - const orgId = useOrganizationId(); const filter = useFilter({ initialValue: `owner:me`, searchParamsResult, @@ -138,6 +137,7 @@ const useWorkspacesFilter = ({ pagination.goToPage(1); }, }); + const permissions = usePermissions(); const canFilterByUser = permissions.viewDeploymentValues; const userMenu = useUserFilterMenu({ @@ -146,12 +146,15 @@ const useWorkspacesFilter = ({ filter.update({ ...filter.values, owner: option?.value }), enabled: canFilterByUser, }); + + const orgId = useOrganizationId(); const templateMenu = useTemplateFilterMenu({ orgId, value: filter.values.template, onChange: (option) => filter.update({ ...filter.values, template: option?.value }), }); + const statusMenu = useStatusFilterMenu({ value: filter.values.status, onChange: (option) => From 92006aa583e60057464cbbc8e23c8485698ca4df Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 13 Sep 2023 21:29:59 +0000 Subject: [PATCH 2/9] refactor: Clean up some filter logic --- site/src/components/Filter/filter.tsx | 24 +++++---- .../pages/WorkspacesPage/filter/filter.tsx | 51 +++++++++++++------ 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 4b8b71f42ff37..9ff4e46b59f7d 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -130,6 +130,18 @@ export const MenuSkeleton = () => ( ); +type FilterProps = { + filter: ReturnType; + skeleton: ReactNode; + isLoading: boolean; + learnMoreLink: string; + learnMoreLabel2?: string; + learnMoreLink2?: string; + error?: unknown; + options?: ReactNode; + presets: PresetFilter[]; +}; + export const Filter = ({ filter, isLoading, @@ -140,17 +152,7 @@ export const Filter = ({ learnMoreLabel2, learnMoreLink2, presets, -}: { - filter: ReturnType; - skeleton: ReactNode; - isLoading: boolean; - learnMoreLink: string; - learnMoreLabel2?: string; - learnMoreLink2?: string; - error?: unknown; - options?: ReactNode; - presets: PresetFilter[]; -}) => { +}: FilterProps) => { const shouldDisplayError = hasError(error) && isApiValidationError(error); const hasFilterQuery = filter.query !== ""; const [searchQuery, setSearchQuery] = useState(filter.query); diff --git a/site/src/pages/WorkspacesPage/filter/filter.tsx b/site/src/pages/WorkspacesPage/filter/filter.tsx index 355598a09c107..b1fe3edb677d3 100644 --- a/site/src/pages/WorkspacesPage/filter/filter.tsx +++ b/site/src/pages/WorkspacesPage/filter/filter.tsx @@ -18,9 +18,22 @@ import { UserFilterMenu, UserMenu } from "components/Filter/UserFilter"; import { workspaceFilterQuery } from "utils/filters"; import { docs } from "utils/docs"; -const PRESET_FILTERS = [ - { query: workspaceFilterQuery.me, name: "My workspaces" }, - { query: workspaceFilterQuery.all, name: "All workspaces" }, +type FilterPreset = { + query: string; + name: string; +}; + +// Can't use as const declarations to make arrays deep readonly because that +// interferes with the type contracts for Filter +const PRESET_FILTERS: FilterPreset[] = [ + { + query: workspaceFilterQuery.me, + name: "My workspaces", + }, + { + query: workspaceFilterQuery.all, + name: "All workspaces", + }, { query: workspaceFilterQuery.running, name: "Running workspaces", @@ -31,11 +44,16 @@ const PRESET_FILTERS = [ }, ]; -export const WorkspacesFilter = ({ - filter, - error, - menus, -}: { +// Defined outside component so that the array doesn't get reconstructed each render +const PRESETS_WITH_DORMANT: FilterPreset[] = [ + ...PRESET_FILTERS, + { + query: workspaceFilterQuery.dormant, + name: "Dormant workspaces", + }, +]; + +type WorkspaceFilterProps = { filter: ReturnType; error?: unknown; menus: { @@ -43,14 +61,15 @@ export const WorkspacesFilter = ({ template: TemplateFilterMenu; status: StatusFilterMenu; }; -}) => { - const presets = [...PRESET_FILTERS]; - if (useIsWorkspaceActionsEnabled()) { - presets.push({ - query: workspaceFilterQuery.dormant, - name: "Dormant workspaces", - }); - } +}; + +export const WorkspacesFilter = ({ + filter, + error, + menus, +}: WorkspaceFilterProps) => { + const actionsEnabled = useIsWorkspaceActionsEnabled(); + const presets = actionsEnabled ? PRESET_FILTERS : PRESETS_WITH_DORMANT; return ( Date: Thu, 14 Sep 2023 16:39:14 +0000 Subject: [PATCH 3/9] refactor: Create debounce utility hooks --- site/src/hooks/debounce.ts | 87 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 site/src/hooks/debounce.ts diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts new file mode 100644 index 0000000000000..e65bcf49bde05 --- /dev/null +++ b/site/src/hooks/debounce.ts @@ -0,0 +1,87 @@ +import { useCallback, useEffect, useRef, useState } from "react"; + +type useDebouncedFunctionReturn = Readonly<{ + debounced: (...args: Args) => void; + + // Mainly here to make interfacing with useEffect cleanup functions easier + cancelDebounce: () => void; +}>; + +/** + * Creates a debounce function that is resilient to React re-renders, as well as + * a function for canceling a pending debounce. + * + * The returned-out functions will maintain the same memory references, but the + * debounce function will always "see" the most recent versions of the arguments + * passed into the hook, and use them accordingly. + * + * If the debounce time changes while a callback has been queued to fire, the + * callback will be canceled completely. You will need to restart the debounce + * process by calling debounced again. + */ +export function useDebouncedFunction< + // Parameterizing on the args instead of the whole callback function type to + // avoid type contra-variance issues; want to avoid need for type assertions + Args extends unknown[] = unknown[], +>( + callback: (...args: Args) => void | Promise, + debounceTimeMs: number, +): useDebouncedFunctionReturn { + const timeoutIdRef = useRef(null); + const cancelDebounce = useCallback(() => { + // Clearing timeout because, even though hot-swapping the timeout value + // while keeping any active debounced functions running was possible, it + // seemed like it'd be ripe for bugs. Can redesign the logic if that ends up + // becoming an actual need down the line. + if (timeoutIdRef.current !== null) { + window.clearTimeout(timeoutIdRef.current); + } + + timeoutIdRef.current = null; + }, []); + + const debounceTimeRef = useRef(debounceTimeMs); + useEffect(() => { + cancelDebounce(); + debounceTimeRef.current = debounceTimeMs; + }, [cancelDebounce, debounceTimeMs]); + + const callbackRef = useRef(callback); + useEffect(() => { + callbackRef.current = callback; + }, [callback]); + + // Returned-out function will always be synchronous, even if the callback arg + // is async. Seemed dicey to try awaiting a genericized operation that can and + // will likely be canceled repeatedly + const debounced = useCallback( + (...args: Args): void => { + cancelDebounce(); + + timeoutIdRef.current = window.setTimeout( + () => void callbackRef.current(...args), + debounceTimeRef.current, + ); + }, + [cancelDebounce], + ); + + return { debounced, cancelDebounce } as const; +} + +export function useDebouncedValue( + value: T, + debounceTimeMs: number, +): T { + const [debouncedValue, setDebouncedValue] = useState(value); + + useEffect(() => { + const timeoutId = window.setTimeout(() => { + setDebouncedValue(value); + }, debounceTimeMs); + + return () => window.clearTimeout(timeoutId); + }, [value, debounceTimeMs]); + + return debouncedValue; +} From fec4384a86935d96e3a4a2ffb7c79801a07d70d3 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 14 Sep 2023 20:07:11 +0000 Subject: [PATCH 4/9] docs: Clean up comments for clarity --- site/src/hooks/debounce.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index e65bcf49bde05..29330383f4971 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -17,11 +17,11 @@ type useDebouncedFunctionReturn = Readonly<{ * * If the debounce time changes while a callback has been queued to fire, the * callback will be canceled completely. You will need to restart the debounce - * process by calling debounced again. + * process by calling the returned-out function again. */ export function useDebouncedFunction< // Parameterizing on the args instead of the whole callback function type to - // avoid type contra-variance issues; want to avoid need for type assertions + // avoid type contra-variance issues Args extends unknown[] = unknown[], >( callback: (...args: Args) => void | Promise, @@ -29,10 +29,6 @@ export function useDebouncedFunction< ): useDebouncedFunctionReturn { const timeoutIdRef = useRef(null); const cancelDebounce = useCallback(() => { - // Clearing timeout because, even though hot-swapping the timeout value - // while keeping any active debounced functions running was possible, it - // seemed like it'd be ripe for bugs. Can redesign the logic if that ends up - // becoming an actual need down the line. if (timeoutIdRef.current !== null) { window.clearTimeout(timeoutIdRef.current); } @@ -69,6 +65,9 @@ export function useDebouncedFunction< return { debounced, cancelDebounce } as const; } +/** + * Takes any value, and returns out a debounced version of it. + */ export function useDebouncedValue( value: T, debounceTimeMs: number, From 2d4d2857449c2e92a11a94f31f875a799559e881 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 14 Sep 2023 20:07:55 +0000 Subject: [PATCH 5/9] fix: Update focus logic to apply for any inner focus --- site/src/components/Filter/filter.tsx | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 9ff4e46b59f7d..4daa14cba7555 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -20,11 +20,11 @@ import { } from "api/errors"; import { useFilterMenu } from "./menu"; import { BaseOption } from "./options"; -import debounce from "just-debounce-it"; import MenuList from "@mui/material/MenuList"; import { Loader } from "components/Loader/Loader"; import Divider from "@mui/material/Divider"; import OpenInNewOutlined from "@mui/icons-material/OpenInNewOutlined"; +import { useDebouncedFunction } from "hooks/debounce"; export type PresetFilter = { name: string; @@ -58,7 +58,7 @@ export const useFilter = ({ } }; - const debounceUpdate = debounce( + const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction( (values: string | FilterValues) => update(values), 500, ); @@ -69,6 +69,7 @@ export const useFilter = ({ query, update, debounceUpdate, + cancelDebounce, values, used, }; @@ -153,19 +154,23 @@ export const Filter = ({ learnMoreLink2, presets, }: FilterProps) => { - const shouldDisplayError = hasError(error) && isApiValidationError(error); - const hasFilterQuery = filter.query !== ""; const [searchQuery, setSearchQuery] = useState(filter.query); - const inputRef = useRef(null); + const textboxInputRef = useRef(null); useEffect(() => { - // We don't want to update this while the user is typing something or has the focus in the input - const isFocused = document.activeElement === inputRef.current; - if (!isFocused) { + // We don't want to update this while the user is typing something or has + // the focus in the input + const hasInnerFocus = + textboxInputRef.current?.contains(document.activeElement) ?? false; + + if (!hasInnerFocus) { setSearchQuery(filter.query); } }, [filter.query]); + const shouldDisplayError = hasError(error) && isApiValidationError(error); + const hasFilterQuery = filter.query !== ""; + return ( { setSearchQuery(e.target.value); filter.debounceUpdate(e.target.value); From 11c06e1f3f01880c4770713024e40707e9c3f35a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 15 Sep 2023 14:30:58 +0000 Subject: [PATCH 6/9] fix: Add onBlur behavior for state syncs --- site/src/components/Filter/filter.tsx | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 4daa14cba7555..fe8d2656e40ad 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -154,17 +154,23 @@ export const Filter = ({ learnMoreLink2, presets, }: FilterProps) => { - const [searchQuery, setSearchQuery] = useState(filter.query); + // Storing local copy of the filter query so that it can be updated more + // aggressively without re-renders rippling out to the rest of the app every + // single time. Exists for performance reasons - not really a good way to + // remove this; render keys would cause the component to remount too often + const [queryCopy, setQueryCopy] = useState(filter.query); const textboxInputRef = useRef(null); + // Conditionally re-syncs the parent and local filter queries useEffect(() => { - // We don't want to update this while the user is typing something or has - // the focus in the input - const hasInnerFocus = + const hasSelfOrInnerFocus = textboxInputRef.current?.contains(document.activeElement) ?? false; - if (!hasInnerFocus) { - setSearchQuery(filter.query); + // This doesn't address all state sync issues - namely, what happens if the + // user removes focus just after this synchronizing effect fires. Also need + // to rely on onBlur behavior as an extra safety measure + if (!hasSelfOrInnerFocus) { + setQueryCopy(filter.query); } }, [filter.query]); @@ -205,12 +211,17 @@ export const Filter = ({ "aria-label": "Filter", name: "query", placeholder: "Search...", - value: searchQuery, + value: queryCopy, ref: textboxInputRef, onChange: (e) => { - setSearchQuery(e.target.value); + setQueryCopy(e.target.value); filter.debounceUpdate(e.target.value); }, + onBlur: () => { + if (queryCopy !== filter.query) { + setQueryCopy(filter.query); + } + }, sx: { borderRadius: "6px", borderTopLeftRadius: 0, From 551780cf529d425ff790029850186bf528397265 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 15 Sep 2023 16:44:03 +0000 Subject: [PATCH 7/9] chore: Add progress for debounce test --- site/src/hooks/debounce.test.ts | 141 ++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 site/src/hooks/debounce.test.ts diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts new file mode 100644 index 0000000000000..d8f6d9424e04c --- /dev/null +++ b/site/src/hooks/debounce.test.ts @@ -0,0 +1,141 @@ +import { renderHook } from "@testing-library/react"; +import { useDebouncedFunction, useDebouncedValue } from "./debounce"; + +beforeAll(() => { + jest.useFakeTimers(); + jest.spyOn(global, "setTimeout"); +}); + +afterAll(() => { + jest.useRealTimers(); + jest.clearAllMocks(); +}); + +// The general approach is to structure the tests from the user's experience, +// but just because these are more abstract, general-purpose hooks, it seemed +// harder to do that. Had to bring in some mocks +function renderDebounceValue(value: T, time: number) { + return renderHook( + ({ value, time }: { value: T; time: number }) => { + return useDebouncedValue(value, time); + }, + { + initialProps: { value, time }, + }, + ); +} + +// Something in the React Testing Library types seems to be losing some of the +// type parameters for the returned-out debounced method. It has much better +// ergonomics/type inference when actually using it in a real React app +function renderDebounceFunction< + Args extends unknown[], + Fn extends (...args: Args) => void | Promise, +>(callback: Fn, time: number) { + return renderHook( + ({ callback, time }: { callback: Fn; time: number }) => { + return useDebouncedFunction(callback, time); + }, + { + initialProps: { callback, time }, + }, + ); +} + +describe(`${useDebouncedValue.name}`, () => { + it("Should immediately return out the exact same value (by reference) on mount", () => { + const value = {}; + const { result } = renderDebounceValue(value, 2000); + + expect(result.current).toBe(value); + expect.hasAssertions(); + }); + + it("Should not immediately resync as the source value changes", async () => { + let value = 0; + const time = 5000; + + const { result, rerender } = renderDebounceValue(value, time); + expect(result.current).toEqual(0); + + for (let i = 1; i <= 5; i++) { + setTimeout(() => { + value++; + rerender({ value, time }); + }, i * 100); + } + + await jest.advanceTimersByTimeAsync(time - 100); + expect(result.current).toEqual(0); + expect.hasAssertions(); + }); + + it("Should resync after specified milliseconds pass with no change to arguments", async () => { + const initialValue = false; + const time = 5000; + + const { result, rerender } = renderDebounceValue(initialValue, time); + expect(result.current).toEqual(false); + + rerender({ value: !initialValue, time }); + await jest.runAllTimersAsync(); + + expect(result.current).toEqual(true); + expect.hasAssertions(); + }); +}); + +describe(`${useDebouncedFunction.name}`, () => { + describe("hook", () => { + it("Should provide stable function references across all renders", () => { + const time = 5000; + const { result, rerender } = renderDebounceFunction(jest.fn(), time); + + const { debounced: oldDebounced, cancelDebounce: oldCancel } = + result.current; + + rerender({ callback: jest.fn(), time }); + const { debounced: newDebounced, cancelDebounce: newCancel } = + result.current; + + expect(oldDebounced).toBe(newDebounced); + expect(oldCancel).toBe(newCancel); + expect.hasAssertions(); + }); + + it.skip("Resets any pending debounces if the timer argument changes", () => { + expect.hasAssertions(); + }); + }); + + describe("debounced function", () => { + it.skip("Should be able to 'see' the most recent arguments across re-renders", () => { + expect.hasAssertions(); + }); + + it.skip("Should reset the debounce timer with repeated calls to the method", () => { + expect.hasAssertions(); + }); + + it.skip("Resolve the debounce after specified milliseconds pass with no other calls", () => { + expect.hasAssertions(); + }); + }); + + describe("cancelDebounce function", () => { + it("Should be able to cancel a pending debounce at any time", async () => { + let count = 0; + const { result } = renderDebounceFunction(() => { + count++; + }, 2000); + + const { debounced, cancelDebounce } = result.current; + debounced(); + cancelDebounce(); + + await jest.runAllTimersAsync(); + expect(count).toEqual(0); + expect.hasAssertions(); + }); + }); +}); From 3d028a7469049511a56fa855b3346daba2df87c5 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 15 Sep 2023 17:50:53 +0000 Subject: [PATCH 8/9] chore: Finish tests for debounce hooks --- site/src/hooks/debounce.test.ts | 102 ++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 25 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index d8f6d9424e04c..5a8ed78c026aa 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,10 +11,10 @@ afterAll(() => { jest.clearAllMocks(); }); -// The general approach is to structure the tests from the user's experience, -// but just because these are more abstract, general-purpose hooks, it seemed -// harder to do that. Had to bring in some mocks -function renderDebounceValue(value: T, time: number) { +// Most UI tests should be structure from the user's experience, but just +// because these are more abstract, general-purpose hooks, it seemed harder to +// do that. Had to bring in some mocks +function renderDebouncedValue(value: T, time: number) { return renderHook( ({ value, time }: { value: T; time: number }) => { return useDebouncedValue(value, time); @@ -25,19 +25,16 @@ function renderDebounceValue(value: T, time: number) { ); } -// Something in the React Testing Library types seems to be losing some of the -// type parameters for the returned-out debounced method. It has much better -// ergonomics/type inference when actually using it in a real React app -function renderDebounceFunction< - Args extends unknown[], - Fn extends (...args: Args) => void | Promise, ->(callback: Fn, time: number) { +function renderDebouncedFunction( + callbackArg: (...args: Args) => void | Promise, + time: number, +) { return renderHook( - ({ callback, time }: { callback: Fn; time: number }) => { + ({ callback, time }: { callback: typeof callbackArg; time: number }) => { return useDebouncedFunction(callback, time); }, { - initialProps: { callback, time }, + initialProps: { callback: callbackArg, time }, }, ); } @@ -45,17 +42,17 @@ function renderDebounceFunction< describe(`${useDebouncedValue.name}`, () => { it("Should immediately return out the exact same value (by reference) on mount", () => { const value = {}; - const { result } = renderDebounceValue(value, 2000); + const { result } = renderDebouncedValue(value, 2000); expect(result.current).toBe(value); expect.hasAssertions(); }); - it("Should not immediately resync as the source value changes", async () => { + it("Should not immediately resync state as the hook re-renders with new value argument", async () => { let value = 0; const time = 5000; - const { result, rerender } = renderDebounceValue(value, time); + const { result, rerender } = renderDebouncedValue(value, time); expect(result.current).toEqual(0); for (let i = 1; i <= 5; i++) { @@ -74,7 +71,7 @@ describe(`${useDebouncedValue.name}`, () => { const initialValue = false; const time = 5000; - const { result, rerender } = renderDebounceValue(initialValue, time); + const { result, rerender } = renderDebouncedValue(initialValue, time); expect(result.current).toEqual(false); rerender({ value: !initialValue, time }); @@ -87,9 +84,9 @@ describe(`${useDebouncedValue.name}`, () => { describe(`${useDebouncedFunction.name}`, () => { describe("hook", () => { - it("Should provide stable function references across all renders", () => { + it("Should provide stable function references across re-renders", () => { const time = 5000; - const { result, rerender } = renderDebounceFunction(jest.fn(), time); + const { result, rerender } = renderDebouncedFunction(jest.fn(), time); const { debounced: oldDebounced, cancelDebounce: oldCancel } = result.current; @@ -103,29 +100,84 @@ describe(`${useDebouncedFunction.name}`, () => { expect.hasAssertions(); }); - it.skip("Resets any pending debounces if the timer argument changes", () => { + it("Resets any pending debounces if the timer argument changes", async () => { + const time = 5000; + let count = 0; + const incrementCount = () => { + count++; + }; + + const { result, rerender } = renderDebouncedFunction( + incrementCount, + time, + ); + + result.current.debounced(); + rerender({ callback: incrementCount, time: time + 1 }); + + await jest.runAllTimersAsync(); + expect(count).toEqual(0); expect.hasAssertions(); }); }); describe("debounced function", () => { - it.skip("Should be able to 'see' the most recent arguments across re-renders", () => { + it("Resolve the debounce after specified milliseconds pass with no other calls", async () => { + let value = false; + const { result } = renderDebouncedFunction(() => { + value = !value; + }, 100); + + result.current.debounced(); + + await jest.runOnlyPendingTimersAsync(); + expect(value).toBe(true); expect.hasAssertions(); }); - it.skip("Should reset the debounce timer with repeated calls to the method", () => { + it("Always uses the most recent callback argument passed in (even if it switches while a debounce is queued)", async () => { + let count = 0; + const time = 500; + + const { result, rerender } = renderDebouncedFunction(() => { + count = 1; + }, time); + + result.current.debounced(); + rerender({ + callback: () => { + count = 9999; + }, + time, + }); + + await jest.runAllTimersAsync(); + expect(count).toEqual(9999); expect.hasAssertions(); }); - it.skip("Resolve the debounce after specified milliseconds pass with no other calls", () => { + it("Should reset the debounce timer with repeated calls to the method", async () => { + let count = 0; + const { result } = renderDebouncedFunction(() => { + count++; + }, 2000); + + for (let i = 0; i < 10; i++) { + setTimeout(() => { + result.current.debounced(); + }, i * 100); + } + + await jest.runAllTimersAsync(); + expect(count).toBe(1); expect.hasAssertions(); }); }); describe("cancelDebounce function", () => { - it("Should be able to cancel a pending debounce at any time", async () => { + it("Should be able to cancel a pending debounce", async () => { let count = 0; - const { result } = renderDebounceFunction(() => { + const { result } = renderDebouncedFunction(() => { count++; }, 2000); From 83c6022265ebdded392de04d9b7c39cc3d137c64 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 15 Sep 2023 18:09:41 +0000 Subject: [PATCH 9/9] docs: Add file description and warning --- site/src/hooks/debounce.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 29330383f4971..38eea91da9933 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -1,3 +1,16 @@ +/** + * @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. + */ import { useCallback, useEffect, useRef, useState } from "react"; type useDebouncedFunctionReturn = Readonly<{