diff --git a/.changeset/react-compiler-hooks.md b/.changeset/react-compiler-hooks.md new file mode 100644 index 00000000000..96ab3b9244b --- /dev/null +++ b/.changeset/react-compiler-hooks.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +useControllableState, useOnOutsideClick: Support automatic memoization with React Compiler. diff --git a/packages/react/script/react-compiler.mjs b/packages/react/script/react-compiler.mjs index c797624c1a3..43d9c7c6e45 100644 --- a/packages/react/script/react-compiler.mjs +++ b/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', ] diff --git a/packages/react/src/hooks/__tests__/useControllableState.test.tsx b/packages/react/src/hooks/__tests__/useControllableState.test.tsx index 2be913011d7..b63333531ec 100644 --- a/packages/react/src/hooks/__tests__/useControllableState.test.tsx +++ b/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(() => {}) diff --git a/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx b/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx index e253ca15ce4..0ef8f1e49b5 100644 --- a/packages/react/src/hooks/__tests__/useOnOutsideClick.test.tsx +++ b/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 = [] + + function Layer({name, preventDefault, children}: PropsWithChildren<{name: string; preventDefault?: boolean}>) { + const containerRef = useRef(null) + useOnOutsideClick({ + containerRef, + onClickOutside(event) { + calls.push(name) + if (preventDefault) { + event.preventDefault() + } + }, + }) + return
{children}
+ } + + function Example({preventDefault}: {preventDefault?: boolean}) { + return ( + + + + + + + ) + } + + const {getByRole, rerender, unmount} = render() + fireEvent.mouseDown(getByRole('button', {name: 'Outside'})) + expect(calls).toEqual(['child', 'parent']) + + calls.length = 0 + rerender() + fireEvent.mouseDown(getByRole('button', {name: 'Outside'})) + expect(calls).toEqual(['child']) + + calls.length = 0 + unmount() + fireEvent.mouseDown(document.body) + expect(calls).toEqual([]) +}) diff --git a/packages/react/src/hooks/useControllableState.ts b/packages/react/src/hooks/useControllableState.ts index cb47004cc68..c0c4878e3ce 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 === false) { internalSetState(value) } stableOnChange.current?.(value) }, - [state], + [state, controlled], ) 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 === false && 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 === true && !controlledValue) { warning( true, 'A component is changing a controlled %s component to be uncontrolled. ' + @@ -100,10 +96,9 @@ export function useControllableState({ 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] } diff --git a/packages/react/src/hooks/useOnOutsideClick.tsx b/packages/react/src/hooks/useOnOutsideClick.tsx index 7bbf341aa71..59934ca3d8b 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, 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( event => { diff --git a/packages/react/src/internal/hooks/useDevOnlyEffect.ts b/packages/react/src/internal/hooks/useDevOnlyEffect.ts index 2f339b4b539..fa65806e28b 100644 --- a/packages/react/src/internal/hooks/useDevOnlyEffect.ts +++ b/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 : () => {}