From 167827daa6441e045add20d1a41cc60afac9db0f Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 15 Nov 2022 06:51:02 -0500 Subject: [PATCH 1/8] add `useUpdateEffect` to `exhaustive-deps` lint rule --- .eslintrc.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.eslintrc.js b/.eslintrc.js index 1d00d8a9031779..408d776aa354de 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -349,7 +349,10 @@ module.exports = { files: [ 'packages/components/src/**/*.[tj]s?(x)' ], excludedFiles: [ ...developmentFiles ], rules: { - 'react-hooks/exhaustive-deps': 'error', + 'react-hooks/exhaustive-deps': [ + 'error', + { additionalHooks: 'useUpdateEffect' }, + ], }, }, { From c1f675aed3db4edd39bd42c4b2858040c02849b8 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 15 Nov 2022 07:55:26 -0500 Subject: [PATCH 2/8] add deps and useMemo for `asButtonGroup` --- .../toggle-group-control/as-button-group.tsx | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx index 72e247659818f5..d4a040580e9f88 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx @@ -12,7 +12,7 @@ import { usePrevious, useResizeObserver, } from '@wordpress/compose'; -import { forwardRef, useRef, useState } from '@wordpress/element'; +import { forwardRef, useRef, useState, useMemo } from '@wordpress/element'; /** * Internal dependencies @@ -47,11 +47,14 @@ function UnforwardedToggleGroupControlAsButtonGroup( 'toggle-group-control-as-button-group' ).toString(); const [ selectedValue, setSelectedValue ] = useState( value ); - const groupContext = { - baseId, - state: selectedValue, - setState: setSelectedValue, - }; + const groupContext = useMemo( + () => ( { + baseId, + state: selectedValue, + setState: setSelectedValue, + } ), + [ baseId, selectedValue ] + ); const previousValue = usePrevious( value ); // Propagate groupContext.state change. @@ -61,14 +64,14 @@ function UnforwardedToggleGroupControlAsButtonGroup( if ( previousValue !== groupContext.state ) { onChange( groupContext.state ); } - }, [ groupContext.state ] ); + }, [ groupContext.state, previousValue, onChange ] ); // Sync incoming value with groupContext.state. useUpdateEffect( () => { if ( value !== groupContext.state ) { groupContext.setState( value ); } - }, [ value ] ); + }, [ value, groupContext ] ); return ( Date: Tue, 15 Nov 2022 10:29:09 -0500 Subject: [PATCH 3/8] udpate deps arrays for `as-radio-group.tsx` --- .../toggle-group-control/as-radio-group.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx index 9f66e964d9d439..8cf75e2266ee6a 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx @@ -61,13 +61,16 @@ function UnforwardedToggleGroupControlAsRadioGroup( if ( previousValue !== radio.state ) { onChange( radio.state ); } - }, [ radio.state ] ); + }, [ radio.state, previousValue, onChange ] ); // Sync incoming value with radio.state. useUpdateEffect( () => { if ( value !== radio.state ) { radio.setState( value ); } + // disable reason: Adding `radio` to the dependency array causes an extra render. + // Memoizing it doesn't look easily doable, so we're disabling the rule for now. + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ value ] ); return ( From cc772883b87abddfce703382fc3cbba309e2bc75 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Tue, 15 Nov 2022 10:35:11 -0500 Subject: [PATCH 4/8] update CHANGELOG --- packages/components/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index d20f381aa8fe9f..2a750563352a0c 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -31,6 +31,8 @@ - `LinkedButton`: remove unnecessary `span` tag ([#46063](https://github.com/WordPress/gutenberg/pull/46063)) - NumberControl: refactor styles/tests/stories to TypeScript, replace fireEvent with user-event ([#45990](https://github.com/WordPress/gutenberg/pull/45990)). - `useBaseField`: Convert to TypeScript ([#45712](https://github.com/WordPress/gutenberg/pull/45712)). +- Add `useUpdateEffect` hook to the previously added `react-hooks/exhuastive-deps` eslint check ([#45771](https://github.com/WordPress/gutenberg/pull/45771)). +- `ToggleGroupControl`: Update `useUpdateEffect` usages to pass `exhaustive-deps` eslint rule ([#45771](https://github.com/WordPress/gutenberg/pull/45771)). ### Documentation From b230269f7285ed791c8f541f1991ac5a34ab2fe3 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Mon, 21 Nov 2022 12:36:42 -0500 Subject: [PATCH 5/8] abstract `radioState` and `setRadioState` for cleaner deps --- .../toggle-group-control/as-radio-group.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx index 8cf75e2266ee6a..483efca45c4cc7 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-radio-group.tsx @@ -64,14 +64,14 @@ function UnforwardedToggleGroupControlAsRadioGroup( }, [ radio.state, previousValue, onChange ] ); // Sync incoming value with radio.state. + const { state: radioState, setState: setRadioState } = radio; useUpdateEffect( () => { - if ( value !== radio.state ) { - radio.setState( value ); + if ( value !== radioState ) { + setRadioState( value ); } - // disable reason: Adding `radio` to the dependency array causes an extra render. - // Memoizing it doesn't look easily doable, so we're disabling the rule for now. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ value ] ); + // setRadioState needs to be listed even if in theory it's supposed to be a + // stable reference — that's an ESLint limitation. + }, [ value, radioState, setRadioState ] ); return ( Date: Wed, 23 Nov 2022 11:28:04 -0500 Subject: [PATCH 6/8] move context into useMemo, and replace getContext with less abstracted variables in the useUpdateEffects --- .../toggle-group-control/as-button-group.tsx | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx index d4a040580e9f88..5af778f458116d 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx @@ -22,7 +22,10 @@ import ToggleGroupControlBackdrop from './toggle-group-control-backdrop'; import ToggleGroupControlContext from '../context'; import { useUpdateEffect } from '../../utils/hooks'; import type { WordPressComponentProps } from '../../ui/context'; -import type { ToggleGroupControlMainControlProps } from '../types'; +import type { + ToggleGroupControlMainControlProps, + ToggleGroupControlContextProps, +} from '../types'; function UnforwardedToggleGroupControlAsButtonGroup( { @@ -47,41 +50,37 @@ function UnforwardedToggleGroupControlAsButtonGroup( 'toggle-group-control-as-button-group' ).toString(); const [ selectedValue, setSelectedValue ] = useState( value ); - const groupContext = useMemo( - () => ( { - baseId, - state: selectedValue, - setState: setSelectedValue, - } ), - [ baseId, selectedValue ] - ); const previousValue = usePrevious( value ); - // Propagate groupContext.state change. + // Propagate selectedValue change. useUpdateEffect( () => { - // Avoid calling onChange if groupContext state changed + // Avoid calling onChange if selectedValue changed // from incoming value. - if ( previousValue !== groupContext.state ) { - onChange( groupContext.state ); + if ( previousValue !== selectedValue ) { + onChange( selectedValue ); } - }, [ groupContext.state, previousValue, onChange ] ); + }, [ selectedValue, previousValue, onChange ] ); - // Sync incoming value with groupContext.state. + // Sync incoming value with selectedValue. useUpdateEffect( () => { - if ( value !== groupContext.state ) { - groupContext.setState( value ); + if ( previousValue !== value ) { + setSelectedValue( value ); } - }, [ value, groupContext ] ); - + }, [ value, previousValue ] ); + // Expose selectedValue getter/setter via context + const groupContext: ToggleGroupControlContextProps = useMemo( + () => ( { + baseId, + state: selectedValue, + setState: setSelectedValue, + isBlock: ! isAdaptiveWidth, + isDeselectable: true, + size, + } ), + [ baseId, selectedValue, isAdaptiveWidth, size ] + ); return ( - + Date: Mon, 28 Nov 2022 11:50:19 -0500 Subject: [PATCH 7/8] replace `useUpdateEffect` implementations with `useControlledValue` --- .../toggle-group-control/as-button-group.tsx | 32 +++++++------------ 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx index 5af778f458116d..0de5543bcc7318 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx @@ -12,7 +12,7 @@ import { usePrevious, useResizeObserver, } from '@wordpress/compose'; -import { forwardRef, useRef, useState, useMemo } from '@wordpress/element'; +import { forwardRef, useRef, useMemo } from '@wordpress/element'; /** * Internal dependencies @@ -20,7 +20,7 @@ import { forwardRef, useRef, useState, useMemo } from '@wordpress/element'; import { View } from '../../view'; import ToggleGroupControlBackdrop from './toggle-group-control-backdrop'; import ToggleGroupControlContext from '../context'; -import { useUpdateEffect } from '../../utils/hooks'; +import { useControlledValue } from '../../utils/hooks'; import type { WordPressComponentProps } from '../../ui/context'; import type { ToggleGroupControlMainControlProps, @@ -49,35 +49,25 @@ function UnforwardedToggleGroupControlAsButtonGroup( ToggleGroupControlAsButtonGroup, 'toggle-group-control-as-button-group' ).toString(); - const [ selectedValue, setSelectedValue ] = useState( value ); const previousValue = usePrevious( value ); - - // Propagate selectedValue change. - useUpdateEffect( () => { - // Avoid calling onChange if selectedValue changed - // from incoming value. - if ( previousValue !== selectedValue ) { - onChange( selectedValue ); - } - }, [ selectedValue, previousValue, onChange ] ); - - // Sync incoming value with selectedValue. - useUpdateEffect( () => { - if ( previousValue !== value ) { - setSelectedValue( value ); - } - }, [ value, previousValue ] ); + const [ selectedValue, setSelectedValue ] = useControlledValue( { + defaultValue: previousValue, + onChange, + value, + } ); // Expose selectedValue getter/setter via context const groupContext: ToggleGroupControlContextProps = useMemo( () => ( { baseId, state: selectedValue, - setState: setSelectedValue, + setState: setSelectedValue as React.Dispatch< + React.SetStateAction< string | number | undefined > + >, isBlock: ! isAdaptiveWidth, isDeselectable: true, size, } ), - [ baseId, selectedValue, isAdaptiveWidth, size ] + [ baseId, selectedValue, setSelectedValue, isAdaptiveWidth, size ] ); return ( From f4a0eca262d532e8f4477d2b50126fcb34294048 Mon Sep 17 00:00:00 2001 From: chad1008 <13856531+chad1008@users.noreply.github.com> Date: Thu, 1 Dec 2022 10:08:11 -0500 Subject: [PATCH 8/8] remove typecasting on `setSelectedValue` --- .../toggle-group-control/as-button-group.tsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx index 0de5543bcc7318..a9e78790711d34 100644 --- a/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx +++ b/packages/components/src/toggle-group-control/toggle-group-control/as-button-group.tsx @@ -60,9 +60,7 @@ function UnforwardedToggleGroupControlAsButtonGroup( () => ( { baseId, state: selectedValue, - setState: setSelectedValue as React.Dispatch< - React.SetStateAction< string | number | undefined > - >, + setState: setSelectedValue, isBlock: ! isAdaptiveWidth, isDeselectable: true, size,