perf(react-compiler): enable controlled state and outside-click hooks

.changeset/react-compiler-hooks.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+useControllableState, useOnOutsideClick: Support automatic memoization with React Compiler.

packages/react/script/react-compiler.mjs

@@ -31,14 +31,11 @@ const unsupportedPatterns = [
   'src/experimental/SelectPanel2/SelectPanel.tsx',
   'src/experimental/UnderlinePanels/UnderlinePanels.test.tsx',
   'src/hooks/useAnchoredPosition.ts',
-  'src/hooks/useControllableState.ts',
   'src/hooks/useFocusTrap.ts',
   'src/hooks/useFocusZone.ts',
   'src/hooks/useOnEscapePress.ts',
-  'src/hooks/useOnOutsideClick.tsx',
   'src/hooks/useResizeObserver.ts',
   'src/internal/hooks/__tests__/useFocus.test.tsx',
-  'src/internal/hooks/useDevOnlyEffect.ts',
   'src/utils/StressTest.tsx',
   'src/utils/react-compiler-runtime.ts',
 ]

packages/react/src/hooks/__tests__/useControllableState.test.tsx

@@ -1,4 +1,4 @@
-import {render} from '@testing-library/react'
+import {act, render, renderHook} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
 import {describe, expect, test, vi} from 'vitest'
 import type React from 'react'
@@ -20,6 +20,39 @@ describe('useControllableState', () => {
     expect(input).toHaveValue('test')
   })
 
+  test('keeps the setter stable while using the latest onChange callback', () => {
+    const initialOnChange = vi.fn()
+    const latestOnChange = vi.fn()
+    const {result, rerender} = renderHook(
+      ({onChange}) => {
+        return useControllableState({
+          defaultValue: '',
+          onChange,
+        })
+      },
+      {
+        initialProps: {
+          onChange: initialOnChange,
+        },
+      },
+    )
+    const setValue = result.current[1]
+
+    rerender({
+      onChange: latestOnChange,
+    })
+
+    expect(result.current[1]).toBe(setValue)
+
+    act(() => {
+      setValue('updated')
+    })
+
+    expect(result.current[0]).toBe('updated')
+    expect(initialOnChange).not.toHaveBeenCalled()
+    expect(latestOnChange).toHaveBeenCalledExactlyOnceWith('updated')
+  })
+
   test('controlled to uncontrolled', async () => {
     const error = vi.spyOn(console, 'error').mockImplementation(() => {})
     const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})

packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx

@@ -1,6 +1,6 @@
-import {render} from '@testing-library/react'
+import {fireEvent, render} from '@testing-library/react'
 import userEvent from '@testing-library/user-event'
-import {useRef} from 'react'
+import {StrictMode, useRef, type PropsWithChildren} from 'react'
 import {it, expect, vi} from 'vitest'
 import {useOnOutsideClick} from '../../hooks/useOnOutsideClick'
 
@@ -56,3 +56,46 @@ it('should not call function when clicking inside container', async () => {
   await user.click(getByText('content'))
   expect(mockFunction).toHaveBeenCalledTimes(0)
 })
+
+it('preserves nested handler priority after rerendering in StrictMode', () => {
+  const calls: Array<string> = []
+
+  function Layer({name, preventDefault, children}: PropsWithChildren<{name: string; preventDefault?: boolean}>) {
+    const containerRef = useRef<HTMLDivElement>(null)
+    useOnOutsideClick({
+      containerRef,
+      onClickOutside(event) {
+        calls.push(name)
+        if (preventDefault) {
+          event.preventDefault()
+        }
+      },
+    })
+    return <div ref={containerRef}>{children}</div>
+  }
+
+  function Example({preventDefault}: {preventDefault?: boolean}) {
+    return (
+      <StrictMode>
+        <button type="button">Outside</button>
+        <Layer name="parent">
+          <Layer name="child" preventDefault={preventDefault} />
+        </Layer>
+      </StrictMode>
+    )
+  }
+
+  const {getByRole, rerender, unmount} = render(<Example />)
+  fireEvent.mouseDown(getByRole('button', {name: 'Outside'}))
+  expect(calls).toEqual(['child', 'parent'])
+
+  calls.length = 0
+  rerender(<Example preventDefault />)
+  fireEvent.mouseDown(getByRole('button', {name: 'Outside'}))
+  expect(calls).toEqual(['child'])
+
+  calls.length = 0
+  unmount()
+  fireEvent.mouseDown(document.body)
+  expect(calls).toEqual([])
+})

packages/react/src/hooks/useControllableState.ts

@@ -40,17 +40,13 @@ export function useControllableState<T>({
   onChange,
 }: ControllableStateOptions<T>): [T, React.Dispatch<React.SetStateAction<T>>] {
   const [state, internalSetState] = React.useState(value ?? defaultValue)
-  const controlled = React.useRef<boolean | null>(null)
+  const [controlled] = React.useState(value !== undefined)
   const stableOnChange = React.useRef(onChange)
 
   React.useEffect(() => {
     stableOnChange.current = onChange
   })
 
-  if (controlled.current === null) {
-    controlled.current = value !== undefined
-  }
-
   const setState = React.useCallback(
     (stateOrUpdater: T | ((prevState: T) => T)) => {
       const value =
@@ -59,13 +55,13 @@ export function useControllableState<T>({
             stateOrUpdater(state)
           : stateOrUpdater
 
-      if (controlled.current === false) {
+      if (controlled === false) {
         internalSetState(value)
       }
 
       stableOnChange.current?.(value)
     },
-    [state],
+    [state, controlled],
   )
 
   React.useEffect(() => {
@@ -74,7 +70,7 @@ export function useControllableState<T>({
     // Uncontrolled -> Controlled
     // If the component prop is uncontrolled, the prop value should be undefined
     // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
-    if (controlled.current === false && controlledValue) {
+    if (controlled === false && controlledValue) {
       warning(
         true,
         'A component is changing an uncontrolled %s component to be controlled. ' +
@@ -89,7 +85,7 @@ export function useControllableState<T>({
     // Controlled -> Uncontrolled
     // If the component prop is controlled, the prop value should be defined
     // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
-    if (controlled.current === true && !controlledValue) {
+    if (controlled === true && !controlledValue) {
       warning(
         true,
         'A component is changing a controlled %s component to be uncontrolled. ' +
@@ -100,10 +96,9 @@ export function useControllableState<T>({
         name,
       )
     }
-  }, [name, value])
+  }, [name, value, controlled])
 
-  // eslint-disable-next-line react-hooks/refs
-  if (controlled.current === true) {
+  if (controlled === true) {
     return [value as T, setState]
   }
 

packages/react/src/hooks/useOnOutsideClick.tsx

@@ -1,5 +1,5 @@
 import type React from 'react'
-import {useEffect, useCallback, useMemo} from 'react'
+import {useEffect, useCallback, useState} from 'react'
 
 export type TouchOrMouseEvent = MouseEvent | TouchEvent
 type TouchOrMouseEventCallback = (event: TouchOrMouseEvent) => boolean | undefined
@@ -42,7 +42,11 @@ function deregister(id: number) {
 let handlerId = 0
 
 export const useOnOutsideClick = ({containerRef, ignoreClickRefs, onClickOutside}: UseOnOutsideClickSettings) => {
-  const id = useMemo(() => handlerId++, [])
+  const [id] = useState(() => {
+    const id = handlerId
+    handlerId += 1
+    return id
+  })
 
   const handler = useCallback<TouchOrMouseEventCallback>(
     event => {

packages/react/src/internal/hooks/useDevOnlyEffect.ts

@@ -1,11 +1,8 @@
 import {useEffect} from 'react'
 
 /**
- * Runs an effect only in development. Wrapping `useEffect` in a regular hook
- * with an outer `__DEV__` guard keeps the production cost to zero (the entire
- * call is dropped by the consumer's `process.env.NODE_ENV` replacement) while
- * centralising the `eslint-disable react-hooks/rules-of-hooks` to one place
- * instead of every call site.
+ * Runs an effect only in development. Selecting the implementation at module
+ * initialization avoids conditional Hook calls and uses a no-op in production.
  *
  * `exhaustive-deps` is wired up to also check call sites of this hook via
  * `additionalEffectHooks` in `eslint.config.mjs`, so callers get the same
@@ -14,10 +11,4 @@ import {useEffect} from 'react'
  * @param effect The effect callback to run in development.
  * @param deps Dependency list, same semantics as `useEffect`.
  */
-export const useDevOnlyEffect = (effect: React.EffectCallback, deps?: React.DependencyList) => {
-  if (__DEV__) {
-    // Forwarding wrapper; deps lint applies at call sites.
-    // eslint-disable-next-line react-hooks/rules-of-hooks
-    useEffect(effect, deps)
-  }
-}
+export const useDevOnlyEffect: typeof useEffect = __DEV__ ? useEffect : () => {}