perf(hooks): stabilize Escape and outside-click callbacks

.changeset/stable-event-hook-callbacks.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+useOnEscapePress, useOnOutsideClick: Improve performance when callbacks change between renders.

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<string> = []
+
+  function Layer({name, preventDefault, children}: PropsWithChildren<{name: string; preventDefault?: boolean}>) {
+    useOnEscapePress(event => {
+      calls.push(name)
+      if (preventDefault) {
+        event.preventDefault()
+      }
+    })
+    return <div>{children}</div>
+  }
+
+  function Example({preventDefault}: {preventDefault?: boolean}) {
+    return (
+      <StrictMode>
+        <Layer name="parent">
+          <Layer name="child" preventDefault={preventDefault} />
+        </Layer>
+      </StrictMode>
+    )
+  }
+
+  const {rerender, unmount} = render(<Example />)
+  fireEvent.keyDown(document.body, {key: 'Escape'})
+  expect(calls).toEqual(['child', 'parent'])
+
+  calls.length = 0
+  rerender(<Example preventDefault />)
+  fireEvent.keyDown(document.body, {key: 'Escape'})
+  expect(calls).toEqual(['child'])
+
+  calls.length = 0
+  unmount()
+  fireEvent.keyDown(document.body, {key: 'Escape'})
+  expect(calls).toEqual([])
+})

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(<Component callback={firstCallback} />)
+  const addListener = vi.spyOn(document, 'addEventListener')
+  const removeListener = vi.spyOn(document, 'removeEventListener')
+
+  rerender(<Component callback={nextCallback} />)
+  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<string> = []
 

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<KeyboardEventCallback>(
-    event => {
-      if (event.key === 'Escape') escapeCallback(event)
-    },
-    [escapeCallback],
-  )
+  const handler = useEffectCallback<KeyboardEventCallback>(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)

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<TouchOrMouseEventCallback>(
-    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<TouchOrMouseEventCallback>(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) {