diff --git a/.changeset/stable-event-hook-callbacks.md b/.changeset/stable-event-hook-callbacks.md new file mode 100644 index 00000000000..1bf005ca896 --- /dev/null +++ b/.changeset/stable-event-hook-callbacks.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +useOnEscapePress, useOnOutsideClick: Improve performance when callbacks change between renders. diff --git a/packages/react/src/hooks/__tests__/useOnEscapePress.test.tsx b/packages/react/src/hooks/__tests__/useOnEscapePress.test.tsx index bfe261bd5da..e247e34d746 100644 --- a/packages/react/src/hooks/__tests__/useOnEscapePress.test.tsx +++ b/packages/react/src/hooks/__tests__/useOnEscapePress.test.tsx @@ -1,7 +1,12 @@ -import {render, fireEvent} from '@testing-library/react' -import {it, expect, vi} from 'vitest' +import {render, renderHook, fireEvent} from '@testing-library/react' +import {StrictMode, useCallback, type PropsWithChildren} from 'react' +import {afterAll, it, expect, vi} from 'vitest' import {useOnEscapePress} from '../../hooks/useOnEscapePress' +afterAll(() => { + vi.restoreAllMocks() +}) + it('should call function when user presses escape', () => { const functionToCall = vi.fn() const Component = () => { @@ -14,3 +19,128 @@ it('should call function when user presses escape', () => { fireEvent.keyDown(domNode, {key: 'Escape', code: 'Escape', keyCode: 27, charCode: 27}) expect(functionToCall).toHaveBeenCalledTimes(1) }) + +it('updates a memoized callback without replacing the document listener', () => { + const onEscape = vi.fn() + const {rerender, unmount} = renderHook( + ({value}) => { + const callback = useCallback(() => { + onEscape(value) + }, [value]) + useOnEscapePress(callback) + }, + { + initialProps: { + value: 0, + }, + }, + ) + const addListener = vi.spyOn(document, 'addEventListener') + const removeListener = vi.spyOn(document, 'removeEventListener') + + rerender({ + value: 1, + }) + fireEvent.keyDown(document.body, {key: 'Enter'}) + expect(onEscape).not.toHaveBeenCalled() + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(onEscape).toHaveBeenCalledExactlyOnceWith(1) + expect(addListener).not.toHaveBeenCalledWith('keydown', expect.any(Function)) + expect(removeListener).not.toHaveBeenCalledWith('keydown', expect.any(Function)) + + unmount() + expect(removeListener).toHaveBeenCalledWith('keydown', expect.any(Function)) + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(onEscape).toHaveBeenCalledTimes(1) +}) + +it('preserves caller-controlled callback dependencies', () => { + const onEscape = vi.fn() + const {rerender} = renderHook( + ({value, dependency}) => { + useOnEscapePress(() => { + onEscape(value) + }, [dependency]) + }, + { + initialProps: { + value: 0, + dependency: 0, + }, + }, + ) + + rerender({ + value: 1, + dependency: 0, + }) + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(onEscape).toHaveBeenLastCalledWith(0) + + rerender({ + value: 2, + dependency: 1, + }) + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(onEscape).toHaveBeenLastCalledWith(2) +}) + +it('preserves an explicitly empty callback dependency list', () => { + const onEscape = vi.fn() + const {rerender} = renderHook( + ({value}) => { + useOnEscapePress(() => { + onEscape(value) + }, []) + }, + { + initialProps: { + value: 0, + }, + }, + ) + + rerender({ + value: 1, + }) + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(onEscape).toHaveBeenCalledExactlyOnceWith(0) +}) + +it('preserves nested handler priority after rerendering in StrictMode', () => { + const calls: Array = [] + + function Layer({name, preventDefault, children}: PropsWithChildren<{name: string; preventDefault?: boolean}>) { + useOnEscapePress(event => { + calls.push(name) + if (preventDefault) { + event.preventDefault() + } + }) + return
{children}
+ } + + function Example({preventDefault}: {preventDefault?: boolean}) { + return ( + + + + + + ) + } + + const {rerender, unmount} = render() + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(calls).toEqual(['child', 'parent']) + + calls.length = 0 + rerender() + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(calls).toEqual(['child']) + + calls.length = 0 + unmount() + fireEvent.keyDown(document.body, {key: 'Escape'}) + expect(calls).toEqual([]) +}) diff --git a/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx b/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx index 0ef8f1e49b5..52aae45a33f 100644 --- a/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx +++ b/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx @@ -1,9 +1,13 @@ import {fireEvent, render} from '@testing-library/react' import userEvent from '@testing-library/user-event' import {StrictMode, useRef, type PropsWithChildren} from 'react' -import {it, expect, vi} from 'vitest' +import {afterAll, it, expect, vi} from 'vitest' import {useOnOutsideClick} from '../../hooks/useOnOutsideClick' +afterAll(() => { + vi.restoreAllMocks() +}) + type ComponentProps = { callback: () => void } @@ -57,6 +61,26 @@ it('should not call function when clicking inside container', async () => { expect(mockFunction).toHaveBeenCalledTimes(0) }) +it('updates the callback without replacing the document listener', () => { + const firstCallback = vi.fn() + const nextCallback = vi.fn() + const {rerender, unmount} = render() + const addListener = vi.spyOn(document, 'addEventListener') + const removeListener = vi.spyOn(document, 'removeEventListener') + + rerender() + fireEvent.mouseDown(document.body) + expect(firstCallback).not.toHaveBeenCalled() + expect(nextCallback).toHaveBeenCalledTimes(1) + expect(addListener).not.toHaveBeenCalledWith('mousedown', expect.any(Function), {capture: true}) + expect(removeListener).not.toHaveBeenCalledWith('mousedown', expect.any(Function), {capture: true}) + + unmount() + expect(removeListener).toHaveBeenCalledWith('mousedown', expect.any(Function), {capture: true}) + fireEvent.mouseDown(document.body) + expect(nextCallback).toHaveBeenCalledTimes(1) +}) + it('preserves nested handler priority after rerendering in StrictMode', () => { const calls: Array = [] diff --git a/packages/react/src/hooks/useOnEscapePress.ts b/packages/react/src/hooks/useOnEscapePress.ts index 83be3320924..68bbe28c919 100644 --- a/packages/react/src/hooks/useOnEscapePress.ts +++ b/packages/react/src/hooks/useOnEscapePress.ts @@ -1,4 +1,5 @@ -import {useEffect, useCallback, useMemo} from 'react' +import {useEffect, useCallback, useState} from 'react' +import {useEffectCallback} from '../internal/hooks/useEffectCallback' /** * Calls all handlers in reverse order @@ -42,6 +43,8 @@ let handlerId = 0 * callback. Each callback has the chance to call `.preventDefault()` on the * event to prevent further callbacks. * + * Callback changes do not re-register the document listener. + * * @param callback {(e: KeyboardEvent) => void} The callback that gets executed * when the Escape key is pressed. The KeyboardEvent generated by the Escape * keypress is passed as the only argument. @@ -57,14 +60,17 @@ export const useOnEscapePress = ( // eslint-disable-next-line react-hooks/exhaustive-deps, react-hooks/use-memo const escapeCallback = useCallback(onEscape, callbackDependencies) - const handler = useCallback( - event => { - if (event.key === 'Escape') escapeCallback(event) - }, - [escapeCallback], - ) + const handler = useEffectCallback(event => { + if (event.key === 'Escape') { + escapeCallback(event) + } + }) - const id = useMemo(() => handlerId++, []) + const [id] = useState(() => { + const id = handlerId + handlerId += 1 + return id + }) useEffect(() => { if (Object.keys(registry).length === 0) { document.addEventListener('keydown', handleEscape) diff --git a/packages/react/src/hooks/useOnOutsideClick.tsx b/packages/react/src/hooks/useOnOutsideClick.tsx index 59934ca3d8b..59c007f48f4 100644 --- a/packages/react/src/hooks/useOnOutsideClick.tsx +++ b/packages/react/src/hooks/useOnOutsideClick.tsx @@ -1,5 +1,6 @@ import type React from 'react' -import {useEffect, useCallback, useState} from 'react' +import {useEffect, useState} from 'react' +import {useEffectCallback} from '../internal/hooks/useEffectCallback' export type TouchOrMouseEvent = MouseEvent | TouchEvent type TouchOrMouseEventCallback = (event: TouchOrMouseEvent) => boolean | undefined @@ -48,27 +49,24 @@ export const useOnOutsideClick = ({containerRef, ignoreClickRefs, onClickOutside return id }) - const handler = useCallback( - event => { - // don't call click handler if the mouse event was triggered by an auxiliary button (right click/wheel button/etc) - if (event instanceof MouseEvent && event.button > 0) { - return stopPropagation - } + const handler = useEffectCallback(event => { + // don't call click handler if the mouse event was triggered by an auxiliary button (right click/wheel button/etc) + if (event instanceof MouseEvent && event.button > 0) { + return stopPropagation + } - // don't call handler if the click happened inside of the container - if (containerRef.current?.contains(event.target as Node)) { - return stopPropagation - } + // don't call handler if the click happened inside of the container + if (containerRef.current?.contains(event.target as Node)) { + return stopPropagation + } - // don't call handler if click happened on an ignored ref - if (ignoreClickRefs && ignoreClickRefs.some(({current}) => current?.contains(event.target as Node))) { - return stopPropagation - } + // don't call handler if click happened on an ignored ref + if (ignoreClickRefs && ignoreClickRefs.some(({current}) => current?.contains(event.target as Node))) { + return stopPropagation + } - onClickOutside(event) - }, - [containerRef, ignoreClickRefs, onClickOutside], - ) + onClickOutside(event) + }) useEffect(() => { if (Object.keys(registry).length === 0) {