perf: enable React Compiler for shared hooks

.changeset/react-compiler-hooks.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Hooks: Enable React Compiler support for shared hook internals.

packages/react/src/hooks/useAnchoredPosition.ts

@@ -4,6 +4,7 @@ import type {AnchorPosition, PositionSettings} from '@primer/behaviors'
 import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
 import {useResizeObserver} from './useResizeObserver'
 import useLayoutEffect from '../utils/useIsomorphicLayoutEffect'
+import {useEffectCallback} from '../internal/hooks/useEffectCallback'
 
 /**
  * Returns all scrollable ancestor elements of the given element, plus the window.
@@ -83,37 +84,33 @@ export function useAnchoredPosition(
     return heightUpdated
   }
 
-  const updatePosition = React.useCallback(
-    () => {
-      if (!enabled) return
-      if (floatingElementRef.current instanceof Element && anchorElementRef.current instanceof Element) {
-        const newPosition = getAnchoredPosition(floatingElementRef.current, anchorElementRef.current, settings)
-        setPosition(prev => {
-          if (settings?.pinPosition && topPositionChanged(prev, newPosition)) {
-            const anchorTop = anchorElementRef.current?.getBoundingClientRect().top ?? 0
-            const elementStillFitsOnTop = anchorTop > (floatingElementRef.current?.clientHeight ?? 0)
-
-            if (elementStillFitsOnTop && updateElementHeight()) {
-              return prev
-            }
+  const updatePosition = useEffectCallback(() => {
+    if (!enabled) return
+    if (floatingElementRef.current instanceof Element && anchorElementRef.current instanceof Element) {
+      const newPosition = getAnchoredPosition(floatingElementRef.current, anchorElementRef.current, settings)
+      setPosition(prev => {
+        if (settings?.pinPosition && topPositionChanged(prev, newPosition)) {
+          const anchorTop = anchorElementRef.current?.getBoundingClientRect().top ?? 0
+          const elementStillFitsOnTop = anchorTop > (floatingElementRef.current?.clientHeight ?? 0)
+
+          if (elementStillFitsOnTop && updateElementHeight()) {
+            return prev
           }
+        }
 
-          if (prev && prev.anchorSide === newPosition.anchorSide) {
-            // if the position hasn't changed, don't update
-            savedOnPositionChange.current?.(newPosition)
-          }
+        if (prev && prev.anchorSide === newPosition.anchorSide) {
+          // if the position hasn't changed, don't update
+          savedOnPositionChange.current?.(newPosition)
+        }
 
-          return newPosition
-        })
-      } else {
-        setPosition(undefined)
-        savedOnPositionChange.current?.(undefined)
-      }
-      setPrevHeight(floatingElementRef.current?.clientHeight)
-    },
-    // eslint-disable-next-line react-hooks/exhaustive-deps, react-hooks/use-memo
-    [floatingElementRef, anchorElementRef, enabled, ...dependencies],
-  )
+        return newPosition
+      })
+    } else {
+      setPosition(undefined)
+      savedOnPositionChange.current?.(undefined)
+    }
+    setPrevHeight(floatingElementRef.current?.clientHeight)
+  })
 
   useLayoutEffect(() => {
     savedOnPositionChange.current = settings?.onPositionChange
@@ -130,7 +127,7 @@ export function useAnchoredPosition(
       hasMountedRef.current = true
       updatePosition()
     }
-  }, [updatePosition, floatingElementRef, anchorElementRef])
+  }, [updatePosition, floatingElementRef, anchorElementRef, enabled, dependencies])
 
   React.useEffect(() => {
     if (!hasMountedRef.current) {

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) {
         internalSetState(value)
       }
 
       stableOnChange.current?.(value)
     },
-    [state],
+    [controlled, state],
   )
 
   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 && 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 && !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])
+  }, [controlled, name, value])
 
-  // eslint-disable-next-line react-hooks/refs
-  if (controlled.current === true) {
+  if (controlled) {
     return [value as T, setState]
   }
 

packages/react/src/hooks/useFocusTrap.ts

@@ -2,6 +2,7 @@
 import {focusTrap} from '@primer/behaviors'
 import {useProvidedRefOrCreate} from './useProvidedRefOrCreate'
 import {useOnOutsideClick} from './useOnOutsideClick'
+import {useEffectCallback} from '../internal/hooks/useEffectCallback'
 
 export interface FocusTrapHookSettings {
   /**
@@ -58,19 +59,21 @@
   const disabled = settings?.disabled
   const abortController = React.useRef<AbortController>()
   const previousFocusedElement = React.useRef<Element | null>(null)
+  const shouldRestoreFocusRef = React.useRef(true)
 
   // If we are enabling a focus trap and haven't already stored the previously focused element
   // go ahead an do that so we can restore later when the trap is disabled.
-  // eslint-disable-next-line react-hooks/refs
-  if (!previousFocusedElement.current && !disabled) {
-    previousFocusedElement.current = document.activeElement
-  }
+  React.useEffect(() => {
+    if (!previousFocusedElement.current && !disabled) {
+      previousFocusedElement.current = document.activeElement
+    }
+  }, [disabled])
 
   // This function removes the event listeners that enable the focus trap and restores focus
   // to the previously-focused element (if necessary).
-  function disableTrap() {
+  const disableTrap = useEffectCallback(() => {
     abortController.current?.abort()
-    if (settings?.allowOutsideClick && outsideClicked) {
+    if (!shouldRestoreFocusRef.current || (settings?.allowOutsideClick && outsideClicked)) {
       return
     }
     if (settings?.returnFocusRef && settings.returnFocusRef.current instanceof HTMLElement) {
@@ -79,32 +82,27 @@
       previousFocusedElement.current.focus()
       previousFocusedElement.current = null
     }
-  }
+  })
 
-  React.useEffect(
-    () => {
-      if (containerRef.current instanceof HTMLElement) {
-        if (!disabled) {
-          abortController.current = focusTrap(containerRef.current, initialFocusRef.current ?? undefined)
-          return () => {
-            disableTrap()
-          }
-        } else {
+  React.useEffect(() => {
+    if (containerRef.current instanceof HTMLElement) {
+      if (!disabled) {
+        shouldRestoreFocusRef.current = true
+        abortController.current = focusTrap(containerRef.current, initialFocusRef.current ?? undefined)
+        return () => {
           disableTrap()
         }
+      } else {
+        disableTrap()
       }
-    },
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    [containerRef, initialFocusRef, disabled, ...dependencies],
-  )
+    }
+  }, [containerRef, initialFocusRef, disabled, dependencies, disableTrap])
   useOnOutsideClick({
     containerRef: containerRef as React.RefObject<HTMLDivElement>,
     onClickOutside: () => {
       setOutsideClicked(true)
       if (settings?.allowOutsideClick) {
-        // eslint-disable-next-line react-hooks/immutability
-        if (settings.returnFocusRef) settings.returnFocusRef = undefined
-        settings.restoreFocusOnCleanUp = false
+        shouldRestoreFocusRef.current = false
         abortController.current?.abort()
       }
     },

packages/react/src/hooks/useFocusZone.ts

@@ -47,30 +47,26 @@ export function useFocusZone(
   const disabled = settings.disabled
   const abortController = React.useRef<AbortController>()
 
-  useEffect(
-    () => {
-      if (
-        containerRef.current instanceof HTMLElement &&
-        // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
-        (!useActiveDescendant || activeDescendantControlRef.current instanceof HTMLElement)
-      ) {
-        if (!disabled) {
-          const vanillaSettings: FocusZoneSettings = {
-            ...settings,
-            activeDescendantControl: activeDescendantControlRef.current ?? undefined,
-          }
-          abortController.current = focusZone(containerRef.current, vanillaSettings)
-          return () => {
-            abortController.current?.abort()
-          }
-        } else {
+  useEffect(() => {
+    if (
+      containerRef.current instanceof HTMLElement &&
+      // eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
+      (!useActiveDescendant || activeDescendantControlRef.current instanceof HTMLElement)
+    ) {
+      if (!disabled) {
+        const vanillaSettings: FocusZoneSettings = {
+          ...settings,
+          activeDescendantControl: activeDescendantControlRef.current ?? undefined,
+        }
+        abortController.current = focusZone(containerRef.current, vanillaSettings)
+        return () => {
           abortController.current?.abort()
         }
+      } else {
+        abortController.current?.abort()
       }
-    },
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    [disabled, ...dependencies],
-  )
+    }
+  }, [activeDescendantControlRef, containerRef, disabled, settings, useActiveDescendant, dependencies])
 
   return {containerRef, activeDescendantControlRef}
 }

packages/react/src/hooks/useMenuInitialFocus.ts

@@ -72,7 +72,6 @@ export const useMenuInitialFocus = (
     },
     // we don't want containerRef in dependencies
     // because re-renders to containerRef while it's open should not fire initialMenuFocus
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    [open, openingGesture, anchorRef],
+    [open, openingGesture, anchorRef, containerRef],
   )
 }

packages/react/src/hooks/useOnEscapePress.ts

@@ -1,4 +1,5 @@
-import {useEffect, useCallback, useMemo} from 'react'
+import {useEffect, useCallback, useId} from 'react'
+import {useEffectCallback} from '../internal/hooks/useEffectCallback'
 
 /**
  * Calls all handlers in reverse order
@@ -16,19 +17,16 @@ function handleEscape(event: KeyboardEvent) {
 
 type KeyboardEventCallback = (event: KeyboardEvent) => void
 
-const registry: {[id: number]: KeyboardEventCallback} = {}
+const registry: {[id: string]: KeyboardEventCallback} = {}
 
-function register(id: number, handler: KeyboardEventCallback): void {
+function register(id: string, handler: KeyboardEventCallback): void {
   registry[id] = handler
 }
 
-function deregister(id: number) {
+function deregister(id: string) {
   delete registry[id]
 }
 
-// For auto-incrementing unique identifiers for registered handlers.
-let handlerId = 0
-
 /**
  * Sets up a `keydown` listener on `window.document`. If
  * 1) The pressed key is "Escape", and
@@ -52,10 +50,9 @@ let handlerId = 0
  */
 export const useOnEscapePress = (
   onEscape: (e: KeyboardEvent) => void,
-  callbackDependencies: React.DependencyList = [onEscape],
+  _callbackDependencies: React.DependencyList = [onEscape],
 ): void => {
-  // eslint-disable-next-line react-hooks/exhaustive-deps, react-hooks/use-memo
-  const escapeCallback = useCallback(onEscape, callbackDependencies)
+  const escapeCallback = useEffectCallback(onEscape)
 
   const handler = useCallback<KeyboardEventCallback>(
     event => {
@@ -64,7 +61,7 @@ export const useOnEscapePress = (
     [escapeCallback],
   )
 
-  const id = useMemo(() => handlerId++, [])
+  const id = useId()
   useEffect(() => {
     if (Object.keys(registry).length === 0) {
       document.addEventListener('keydown', handleEscape)

packages/react/src/hooks/useOnOutsideClick.tsx

@@ -1,5 +1,5 @@
 import type React from 'react'
-import {useEffect, useCallback, useMemo} from 'react'
+import {useEffect, useCallback, useId} from 'react'
 
 export type TouchOrMouseEvent = MouseEvent | TouchEvent
 type TouchOrMouseEventCallback = (event: TouchOrMouseEvent) => boolean | undefined
@@ -28,21 +28,18 @@ function handleClick(event: MouseEvent) {
   }
 }
 
-const registry: {[id: number]: TouchOrMouseEventCallback} = {}
+const registry: {[id: string]: TouchOrMouseEventCallback} = {}
 
-function register(id: number, handler: TouchOrMouseEventCallback): void {
+function register(id: string, handler: TouchOrMouseEventCallback): void {
   registry[id] = handler
 }
 
-function deregister(id: number) {
+function deregister(id: string) {
   delete registry[id]
 }
 
-// For auto-incrementing unique identifiers for registered handlers.
-let handlerId = 0
-
 export const useOnOutsideClick = ({containerRef, ignoreClickRefs, onClickOutside}: UseOnOutsideClickSettings) => {
-  const id = useMemo(() => handlerId++, [])
+  const id = useId()
 
   const handler = useCallback<TouchOrMouseEventCallback>(
     event => {

packages/react/src/hooks/useResizeObserver.ts

@@ -61,7 +61,5 @@ export function useResizeObserver<T extends HTMLElement>(
         window.removeEventListener('resize', saveTargetDimensions)
       }
     }
-
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [target?.current, enabled, ...depsArray])
+  }, [target, targetClientRect?.height, targetClientRect?.width, enabled, depsArray])
 }

packages/react/src/hooks/useSafeTimeout.ts

@@ -26,9 +26,10 @@ export default function useSafeTimeout(): {safeSetTimeout: SetTimeout; safeClear
   }, [])
 
   useEffect(() => {
+    const currentTimers = timers.current
+
     return () => {
-      // eslint-disable-next-line react-hooks/exhaustive-deps
-      for (const id of timers.current) {
+      for (const id of currentTimers) {
         clearTimeout(id)
       }
     }