diff --git a/.changeset/yellow-tools-call.md b/.changeset/yellow-tools-call.md new file mode 100644 index 00000000000..0b868ed40d6 --- /dev/null +++ b/.changeset/yellow-tools-call.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +avoid useeffect when syncing theme config diff --git a/package-lock.json b/package-lock.json index 0904d9bc007..974ff76d890 100644 --- a/package-lock.json +++ b/package-lock.json @@ -63203,4 +63203,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/react/src/ThemeProvider.tsx b/packages/react/src/ThemeProvider.tsx index e8647ef6a20..338294e54a2 100644 --- a/packages/react/src/ThemeProvider.tsx +++ b/packages/react/src/ThemeProvider.tsx @@ -4,6 +4,7 @@ import {ThemeProvider as SCThemeProvider} from 'styled-components' import defaultTheme from './theme' import deepmerge from 'deepmerge' import {useId} from './hooks' +import {useSyncedState} from './hooks/useSyncedState' export const defaultColorMode = 'day' const defaultDayScheme = 'light' @@ -66,9 +67,9 @@ export const ThemeProvider: React.FC const {resolvedServerColorMode} = getServerHandoff(uniqueDataId) const resolvedColorModePassthrough = React.useRef(resolvedServerColorMode) - const [colorMode, setColorMode] = React.useState(props.colorMode ?? fallbackColorMode ?? defaultColorMode) - const [dayScheme, setDayScheme] = React.useState(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) - const [nightScheme, setNightScheme] = React.useState(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme) + const [colorMode, setColorMode] = useSyncedState(props.colorMode ?? fallbackColorMode ?? defaultColorMode) + const [dayScheme, setDayScheme] = useSyncedState(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) + const [nightScheme, setNightScheme] = useSyncedState(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme) const systemColorMode = useSystemColorMode() const resolvedColorMode = resolvedColorModePassthrough.current || resolveColorMode(colorMode, systemColorMode) const colorScheme = chooseColorScheme(resolvedColorMode, dayScheme, nightScheme) @@ -101,22 +102,9 @@ export const ThemeProvider: React.FC resolvedColorModePassthrough.current = null } }, - [colorMode, systemColorMode], + [colorMode, systemColorMode, setColorMode], ) - // Update state if props change - React.useEffect(() => { - setColorMode(props.colorMode ?? fallbackColorMode ?? defaultColorMode) - }, [props.colorMode, fallbackColorMode]) - - React.useEffect(() => { - setDayScheme(props.dayScheme ?? fallbackDayScheme ?? defaultDayScheme) - }, [props.dayScheme, fallbackDayScheme]) - - React.useEffect(() => { - setNightScheme(props.nightScheme ?? fallbackNightScheme ?? defaultNightScheme) - }, [props.nightScheme, fallbackNightScheme]) - return ( string), + {isPropUpdateDisabled}: {isPropUpdateDisabled: boolean} = {isPropUpdateDisabled: false}, +) => { + return renderHook(props => useSyncedState(props.initialValue, {isPropUpdateDisabled: props.isPropUpdateDisabled}), { + initialProps: { + initialValue, + isPropUpdateDisabled, + }, + }) +} +test('it renders a default', () => { + const {result} = renderUseSyncedState('default') + expect(result.current[0]).toEqual('default') +}) + +test('it updates state from the internal state setter', () => { + const {result} = renderUseSyncedState('default') + expect(result.current[0]).toEqual('default') + act(() => { + result.current[1]('new value') + }) + expect(result.current[0]).toEqual('new value') +}) + +test('it updates state from the internal state setter with an updater fn', () => { + const {result} = renderUseSyncedState('default') + expect(result.current[0]).toEqual('default') + act(() => { + result.current[1](prev => `${prev} new value`) + }) + expect(result.current[0]).toEqual('default new value') +}) + +test('it updates state from the external state setter', () => { + const {result, rerender} = renderUseSyncedState('default') + expect(result.current[0]).toEqual('default') + + rerender({initialValue: 'new value', isPropUpdateDisabled: false}) + + expect(result.current[0]).toEqual('new value') +}) + +test('it properly handles init functions', () => { + const {result, rerender} = renderUseSyncedState(() => 'default') + expect(result.current[0]).toEqual('default') + + rerender({initialValue: () => 'new value', isPropUpdateDisabled: false}) + + expect(result.current[0]).toEqual('new value') +}) + +test('it does not update from props if disabled', () => { + const {result, rerender} = renderUseSyncedState('default', {isPropUpdateDisabled: true}) + expect(result.current[0]).toEqual('default') + + rerender({initialValue: 'new value', isPropUpdateDisabled: true}) + + expect(result.current[0]).toEqual('default') +}) diff --git a/packages/react/src/hooks/useSyncedState.tsx b/packages/react/src/hooks/useSyncedState.tsx new file mode 100644 index 00000000000..748f442bf22 --- /dev/null +++ b/packages/react/src/hooks/useSyncedState.tsx @@ -0,0 +1,28 @@ +import {useState} from 'react' + +/** + * When the value that initialized the state changes + * this hook will update the state to the new value, immediately. + * + * This uses an Object.is comparison to determine if the value has changed by default + * + * If you use a non-primitive value as the initial value, you should provide a custom isEqual function + * + * This is adapted almost directly from https://beta.reactjs.org/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes + */ + +export const useSyncedState = ( + initialValue: T | (() => T), + {isPropUpdateDisabled = false, isEqual = Object.is} = {}, +) => { + const [state, setState] = useState(initialValue) + const [previous, setPrevious] = useState(initialValue) + + const nextInitialValue = initialValue instanceof Function ? initialValue() : initialValue + if (!isPropUpdateDisabled && !isEqual(previous, nextInitialValue)) { + setPrevious(nextInitialValue) + setState(nextInitialValue) + } + + return [state, setState] as const +}