diff --git a/.changeset/react-compiler-hooks.md b/.changeset/react-compiler-hooks.md new file mode 100644 index 00000000000..9377bdad7b7 --- /dev/null +++ b/.changeset/react-compiler-hooks.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Hooks: Enable React Compiler support for shared hook internals. diff --git a/packages/react/src/hooks/useAnchoredPosition.ts b/packages/react/src/hooks/useAnchoredPosition.ts index 611d1b1e920..5b704d63446 100644 --- a/packages/react/src/hooks/useAnchoredPosition.ts +++ b/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) { diff --git a/packages/react/src/hooks/useControllableState.ts b/packages/react/src/hooks/useControllableState.ts index cb47004cc68..581dc63b56d 100644 --- a/packages/react/src/hooks/useControllableState.ts +++ b/packages/react/src/hooks/useControllableState.ts @@ -40,17 +40,13 @@ export function useControllableState({ onChange, }: ControllableStateOptions): [T, React.Dispatch>] { const [state, internalSetState] = React.useState(value ?? defaultValue) - const controlled = React.useRef(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({ 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({ // 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({ // 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({ name, ) } - }, [name, value]) + }, [controlled, name, value]) - // eslint-disable-next-line react-hooks/refs - if (controlled.current === true) { + if (controlled) { return [value as T, setState] } diff --git a/packages/react/src/hooks/useFocusTrap.ts b/packages/react/src/hooks/useFocusTrap.ts index 330888be997..2ef81ad9cc0 100644 --- a/packages/react/src/hooks/useFocusTrap.ts +++ b/packages/react/src/hooks/useFocusTrap.ts @@ -2,6 +2,7 @@ import React from 'react' 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 @@ export function useFocusTrap( const disabled = settings?.disabled const abortController = React.useRef() const previousFocusedElement = React.useRef(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 @@ export function useFocusTrap( 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, 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() } }, diff --git a/packages/react/src/hooks/useFocusZone.ts b/packages/react/src/hooks/useFocusZone.ts index 9f5443c2100..3847854b256 100644 --- a/packages/react/src/hooks/useFocusZone.ts +++ b/packages/react/src/hooks/useFocusZone.ts @@ -47,30 +47,26 @@ export function useFocusZone( const disabled = settings.disabled const abortController = React.useRef() - 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} } diff --git a/packages/react/src/hooks/useMenuInitialFocus.ts b/packages/react/src/hooks/useMenuInitialFocus.ts index b28d69b19ba..5b5e3864ead 100644 --- a/packages/react/src/hooks/useMenuInitialFocus.ts +++ b/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], ) } diff --git a/packages/react/src/hooks/useOnEscapePress.ts b/packages/react/src/hooks/useOnEscapePress.ts index 83be3320924..c037145985a 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, 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( 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) diff --git a/packages/react/src/hooks/useOnOutsideClick.tsx b/packages/react/src/hooks/useOnOutsideClick.tsx index 7bbf341aa71..d4462f86724 100644 --- a/packages/react/src/hooks/useOnOutsideClick.tsx +++ b/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( event => { diff --git a/packages/react/src/hooks/useResizeObserver.ts b/packages/react/src/hooks/useResizeObserver.ts index 20636071b8a..780ada8805c 100644 --- a/packages/react/src/hooks/useResizeObserver.ts +++ b/packages/react/src/hooks/useResizeObserver.ts @@ -61,7 +61,5 @@ export function useResizeObserver( window.removeEventListener('resize', saveTargetDimensions) } } - - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [target?.current, enabled, ...depsArray]) + }, [target, targetClientRect?.height, targetClientRect?.width, enabled, depsArray]) } diff --git a/packages/react/src/hooks/useSafeTimeout.ts b/packages/react/src/hooks/useSafeTimeout.ts index 2ee793c24d8..a921509b944 100644 --- a/packages/react/src/hooks/useSafeTimeout.ts +++ b/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) } } diff --git a/packages/react/src/internal/hooks/useDevOnlyEffect.ts b/packages/react/src/internal/hooks/useDevOnlyEffect.ts index 2f339b4b539..d792782965d 100644 --- a/packages/react/src/internal/hooks/useDevOnlyEffect.ts +++ b/packages/react/src/internal/hooks/useDevOnlyEffect.ts @@ -1,23 +1,25 @@ import {useEffect} from 'react' +import {useEffectCallback} from './useEffectCallback' /** * 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. + * with an outer `__DEV__` guard keeps the production cost low while centralising + * the development-only effect behavior instead of repeating it at every call + * site. * * `exhaustive-deps` is wired up to also check call sites of this hook via * `additionalEffectHooks` in `eslint.config.mjs`, so callers get the same * deps lint they would for a plain `useEffect`. * * @param effect The effect callback to run in development. - * @param deps Dependency list, same semantics as `useEffect`. + * @param _deps Dependency list accepted for call-site compatibility. */ -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 = (effect: React.EffectCallback, _deps?: React.DependencyList) => { + const effectCallback = useEffectCallback(effect) + + useEffect(() => { + if (__DEV__) { + return effectCallback() + } + }) } diff --git a/packages/react/src/utils/use-force-update.ts b/packages/react/src/utils/use-force-update.ts index b61e0ca89d3..9fb15dd20a0 100644 --- a/packages/react/src/utils/use-force-update.ts +++ b/packages/react/src/utils/use-force-update.ts @@ -2,6 +2,6 @@ import React from 'react' export const useForceUpdate = () => { - const [, rerender] = React.useState({}) - return React.useCallback(() => rerender({}), []) + const [, forceUpdate] = React.useReducer(count => count + 1, 0) + return forceUpdate }