Components: add useUpdateEffect to exhaustive-deps lint check
#45771
Conversation
|
|
|
Size Change: +20 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
chad1008
added
[Type] Code Quality
|
|
||
| // Sync incoming value with groupContext.state. | ||
| useUpdateEffect( () => { | ||
| if ( value !== groupContext.state ) { | ||
| groupContext.setState( value ); | ||
| } | ||
| }, [ value ] ); | ||
| }, [ value, groupContext ] ); |
There was a problem hiding this comment.
Since groupContext.setState is effectively setSelectedValue, I wonder if we could just swap the call inside the hook to use setSelectedValue instead of groupContext.setState. That way, ESLint should not complain about a missing dependency?
There was a problem hiding this comment.
I don't see any problem with that approach! One thing to bear in mind though is that to actually eliminate the the groupContext dependency, we'd also need to replace groupContext.state with selectedValue. At that point, selectedValue becomes a dependency, so we've kind of come full circle and are still adding a second dependency.
That said, if we feel that selectedValue and setSelectedValue are more readable than the current groupContext values, I'm happy to make that change! 🙂
There was a problem hiding this comment.
Actually, I was thinking that using selectedValue and setSelectedValue fits my mental model better:
valueorselectedValuechange- the hook calls
setSelectedValue - this causes a re-render, which causes the
useMemohook to re-run and updategroupContextwith the latestselectedValue groupContextis not really used internally by the component, but only created in order to pass it to the context provider, for child components to use. This allows also to move all context props underuseMemo, in order to avoid creating a new context object every render
Something like this
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 d4a040580e..3708f9b7bb 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,39 @@ 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 changes.
useUpdateEffect( () => {
- // Avoid calling onChange if groupContext state changed
+ // Avoid calling onChange if the 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 ( value !== selectedValue ) {
+ setSelectedValue( value );
}
- }, [ value, groupContext ] );
+ }, [ value, selectedValue ] );
+
+ // 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 (
- <ToggleGroupControlContext.Provider
- value={ {
- ...groupContext,
- isBlock: ! isAdaptiveWidth,
- isDeselectable: true,
- size,
- } }
- >
+ <ToggleGroupControlContext.Provider value={ groupContext }>
<View
aria-label={ label }
{ ...otherProps }How does the above proposed change look to y'all? (cc @mirka )
There was a problem hiding this comment.
Interestingly, it looks like at least one of our failing tests on this PR is being caused by adding selectedValue (or groupContext under the previous implementation) to the dependency array of the second useUpdateEffect.
It doesn't look like the deselection is actually working. The test clicks the button once, then clicks it again, but the pressed state doesn't look like it's updating. I'll keep investigating.
There was a problem hiding this comment.
Marco's suggestions make perfect sense to me 👍
Looks like the test failure is because the test is doing an uncontrolled usage. Uncontrolled usages would legitimately break if we added selectedValue to the deps array.
The second useEffect is intended to sync incoming value prop changes to the internal component state. (The first useEffect is the inverse — it syncs internal state up to the outer controlled value via onChange.)
We only want the effect to fire when value changes, not when selectedValue changes. So something like this is probably what we want instead:
// Sync incoming value with selectedValue.
useUpdateEffect( () => {
if ( previousValue !== value ) {
setSelectedValue( value );
}
}, [ value, previousValue ] );There was a problem hiding this comment.
ah. yes that makes sense. Thank you!
Changes applied, waiting on CI. 🤞
There was a problem hiding this comment.
Tested these changes on Storybook — it seems like the onChange callback gets called twice every time the value changes
I wonder if using useControlledValue instead of the custom logic would help here with syncing controlled and uncontrolled updates?
There was a problem hiding this comment.
Oh, promising idea. I've pushed an attempt at this change in 3acacad. I had to address a type discrepancy on the setter function, let me know how it looks!
edit: updated commit sha after rebasing
There was a problem hiding this comment.
Okay, update on the useControlledValue approach. It proved to be problematic. Expected behavior is that you click a button once, it gets selected. Click it again, it deselects.
With useControlledValue, the second click doesn't do anything (at least not from a UI perspective). It's the third click that actually deactivates the button and resets the value.
I believe this is happening because useControlledValue looks for the value prop to be undefined, and ToggleGroupControlOptionBase intentionally passes undefined when deselecting. This means useControlledValue sees undefined as the value being passed in, assumes there is no value, and treats it as an uncontrolled component.
We can work around that by having ToggleGroupControlOptionBase pass an empty string instead of undefined, and make one small test fix. Before I push that change though, I wondered what your thoughts on that might be @ciampo @mirka
There was a problem hiding this comment.
I prefer not to have to pass an empty string instead of undefined, since that is rather arbitrary and not a solution that can be adopted in the general case (e.g., what if our state was a number type?). In fact, it's probably worth fixing this limitation in useControlledValue itself (not in this PR).
In our specific case, I feel like the most straightforward fix for the double onChange problem is something like this:
// Propagate selectedValue change.
useUpdateEffect( () => {
// Only call onChange when uncontrolled
if ( previousValue === value ) {
onChange( selectedValue );
}
}, [ selectedValue, previousValue, value, onChange ] );This seems to be working in my brief testing for both controlled/uncontrolled cases, though I think it's worth adding tests. Marco and I were talking about how we might want to start adding a .each() to our component tests so everything is tested in both controlled/uncontrolled cases. @chad1008 Do you want to try that here? It might not fit the scope of this PR, and this PR has been running long, so feel free to defer if it's too high-effort to include here. As long as we are reasonably confident that we haven't regressed, this PR can land.
|
|
||
| // 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. |
There was a problem hiding this comment.
What if we did something like
const setRadioState = radio.setState;
// ...
useUpdateEffect( () => {
if ( value !== radio.state ) {
setRadioState( value );
}
}, [ setRadioState, value ] );That way, the hook will re-run only when setRadioState changes (and not the whole radio object). Furthermore, there's a chance that setState is an actual React useState setter, which means its value will be stable throughout the component's lifetime (although we should check for that)
There was a problem hiding this comment.
Good idea - playing with it got interesting though:
If we go this route, we then need radio.state as a dependency. ESLint wasn't asking for that before, perhaps because it would have been happy with the full radio object?
Anyway - if we add that dependency, we end up (once again) with an additional render whenever a new option is selected, just like we'd get if we added radio directly. I suppose that makes sense, since radio.state is going to change every time.
This leaves me uncertain of the best course forward. Leave the disabling comment for now? Just add radio as the linter originally wanted, rather than introduce a new variable that doesn't actually solve our issue? 🤔
There was a problem hiding this comment.
Good point. Adding those dependencies sounds correct to me, though — we could apply the technique that I mentioned above to both state and setState, and do something like this
Click to expand
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 8cf75e2266..a2fc579d2b 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.
+ }, [ radioState, setRadioState, value ] );
return (
<ToggleGroupControlContext.ProviderWe'd need of course to perform some testing and make sure that this doesn't introduce regressions — but, in theory, the hook is relatively simple and it makes sense to me that we re-run the hook when the radio state updates
There was a problem hiding this comment.
I had a similar thought 🙂 I'll get those changes pushed now!
There was a problem hiding this comment.
Left a couple additional suggestions. On top of that, I think that it would be best to update the as-radio-group.tsx file with the same pattern (using useControlledState for handling controlled/uncontrolled updates, creating the context value separately with useMemo), even if it's not 100% related to original scope of the PR
chad1008
force-pushed
the
enhance/add-useUpdateEffect-exhaustive-deps
branch
2 times, most recently
from
ee82b6c to
e184c9c
Compare
|
Investigating test failures... |
chad1008
force-pushed
the
enhance/add-useUpdateEffect-exhaustive-deps
branch
from
64ef11f to
3acacad
Compare
| }, [ value ] ); | ||
|
|
||
| const [ selectedValue, setSelectedValue ] = useControlledValue( { | ||
| defaultValue: previousValue, |
There was a problem hiding this comment.
I don't think we need this line (and probably we don't need previousValue at all anymore)
| setState: setSelectedValue as React.Dispatch< | ||
| React.SetStateAction< string | number | undefined > | ||
| >, |
There was a problem hiding this comment.
Instead of typecasting setSelectedValue, we can try to type useControlledValue a bit most narrowly using const assertions
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 0de5543bcc..a9e7879071 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,
diff --git a/packages/components/src/utils/hooks/use-controlled-value.ts b/packages/components/src/utils/hooks/use-controlled-value.ts
index 5a3824d465..bb7f3aea80 100644
--- a/packages/components/src/utils/hooks/use-controlled-value.ts
+++ b/packages/components/src/utils/hooks/use-controlled-value.ts
@@ -22,7 +22,7 @@ export function useControlledValue< T >( {
defaultValue,
onChange,
value: valueProp,
-}: Props< T > ): [ T | undefined, ( value: T ) => void ] {
+}: Props< T > ) {
const hasValue = typeof valueProp !== 'undefined';
const initialValue = hasValue ? valueProp : defaultValue;
const [ state, setState ] = useState( initialValue );
@@ -40,5 +40,5 @@ export function useControlledValue< T >( {
setValue = setState;
}
- return [ value, setValue ];
+ return [ value, setValue as typeof setState ] as const;
}Basically:
- in the return statement, we typecast
setValueto be "whatever is the type of thesetStatefunction - we add
as const, which basically tells TypeScript to interpret the type of the return statement as literally as possible
There was a problem hiding this comment.
I can actually open a separate PR with this change, so that this PR stays focused on ToggleGroupControl
There was a problem hiding this comment.
#46164 (merged), we can rebase on top on trunk to include those changes, which should allow to remove the as React.Dispatch< ... type cast
There was a problem hiding this comment.
Thank you for making that change. I'm updating the typecasting now.
…d variables in the useUpdateEffects
|
Something odd I'm finding but haven't been able to sort out yet - with any of these changes, the |
chad1008
force-pushed
the
enhance/add-useUpdateEffect-exhaustive-deps
branch
from
3acacad to
f4a0eca
Compare
|
So! I spent some time investigating this issue — here are my findings. InvestigationThe failing test has a peculiarity: it seems to test the component in a mixed controlled/uncontrolled way:
By applying this same scenario to the Storybook example, I was able to reproduce the same failure in Storybook — basically, since the Here are the changes that I made in Storybook that allow replicating the bugdiff --git a/packages/components/src/toggle-group-control/stories/index.tsx b/packages/components/src/toggle-group-control/stories/index.tsx
index 9978481764..6c21a9548f 100644
--- a/packages/components/src/toggle-group-control/stories/index.tsx
+++ b/packages/components/src/toggle-group-control/stories/index.tsx
@@ -6,7 +6,7 @@ import type { ComponentMeta, ComponentStory } from '@storybook/react';
/**
* WordPress dependencies
*/
-import { useState } from '@wordpress/element';
+// import { useState } from '@wordpress/element';
import { formatLowercase, formatUppercase } from '@wordpress/icons';
/**
@@ -20,7 +20,7 @@ import {
import type {
ToggleGroupControlOptionProps,
ToggleGroupControlOptionIconProps,
- ToggleGroupControlProps,
+ // ToggleGroupControlProps,
} from '../types';
const meta: ComponentMeta< typeof ToggleGroupControl > = {
@@ -43,17 +43,17 @@ const Template: ComponentStory< typeof ToggleGroupControl > = ( {
onChange,
...props
} ) => {
- const [ value, setValue ] =
- useState< ToggleGroupControlProps[ 'value' ] >();
+ // const [ value, setValue ] =
+ // useState< ToggleGroupControlProps[ 'value' ] >();
return (
<ToggleGroupControl
{ ...props }
onChange={ ( ...changeArgs ) => {
- setValue( ...changeArgs );
+ // setValue( ...changeArgs );
onChange?.( ...changeArgs );
} }
- value={ value }
+ // value={ value }
/>
);
};
@@ -133,4 +133,5 @@ export const Deselectable: ComponentStory< typeof ToggleGroupControl > =
Deselectable.args = {
...WithIcons.args,
isDeselectable: true,
+ value: 'uppercase',
};I was then able to make the tests pass by playing around with the See first attempted fixdiff --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 a9e7879071..9b4191f750 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
@@ -9,7 +9,6 @@ import type { ForwardedRef } from 'react';
import {
useMergeRefs,
useInstanceId,
- usePrevious,
useResizeObserver,
} from '@wordpress/compose';
import { forwardRef, useRef, useMemo } from '@wordpress/element';
@@ -49,11 +48,9 @@ function UnforwardedToggleGroupControlAsButtonGroup(
ToggleGroupControlAsButtonGroup,
'toggle-group-control-as-button-group'
).toString();
- const previousValue = usePrevious( value );
const [ selectedValue, setSelectedValue ] = useControlledValue( {
- defaultValue: previousValue,
+ defaultValue: value,
onChange,
- value,
} );
// Expose selectedValue getter/setter via context
const groupContext: ToggleGroupControlContextProps = useMemo(Although that felt more like a hack that avoids the issue, rather than a fix. ConclusionsIt looks like Initial value vs current valueOn A better approach in my opinion would be for consumers of Diff showing how `defaultValue` prop could be used in `ToggleGroupControlAsButtonGroup`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 a9e7879071..8780e1a333 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
@@ -9,7 +9,6 @@ import type { ForwardedRef } from 'react';
import {
useMergeRefs,
useInstanceId,
- usePrevious,
useResizeObserver,
} from '@wordpress/compose';
import { forwardRef, useRef, useMemo } from '@wordpress/element';
@@ -34,6 +33,7 @@ function UnforwardedToggleGroupControlAsButtonGroup(
label,
onChange,
size,
+ defaultValue,
value,
...otherProps
}: WordPressComponentProps<
@@ -49,9 +49,8 @@ function UnforwardedToggleGroupControlAsButtonGroup(
ToggleGroupControlAsButtonGroup,
'toggle-group-control-as-button-group'
).toString();
- const previousValue = usePrevious( value );
const [ selectedValue, setSelectedValue ] = useControlledValue( {
- defaultValue: previousValue,
+ defaultValue: defaultValue as string | number,
onChange,
value,
} );
diff --git a/packages/components/src/toggle-group-control/toggle-group-control/component.tsx b/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
index ebd4893e37..7a35c81a72 100644
--- a/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
+++ b/packages/components/src/toggle-group-control/toggle-group-control/component.tsx
@@ -42,6 +42,7 @@ function UnconnectedToggleGroupControl(
onChange = noop,
size = 'default',
value,
+ defaultValue,
children,
...otherProps
} = useContextSystem( props, 'ToggleGroupControl' );
@@ -81,6 +82,7 @@ function UnconnectedToggleGroupControl(
ref={ forwardedRef }
size={ size }
value={ value }
+ defaultValue={ defaultValue }
/>
</BaseControl>
);Using
|
|
Thank you so much for digging into that @ciampo! I'll await @mirka's thoughts on your full conclusions, but my initial thought thought is that I also think it makes sense to pass an empty string instead of undefined (side note, is it wrong that I'm this excited that we had the same thought there? 😆 In addition to that, a quick question for my own edification - you've presented two options on the unit test (fully controlled vs. fully uncontrolled). Would it make sense to test both, or is that overkill? |
Yup, let's wait for Lena's thoughts!
I would personally like to test both controlled and uncontrolled, as testing for both behaviours can help us flag bugs and inconsistencies (like in this PR). |
|
My thoughts are basically this — the most important point being that I'm reluctant to adopt the "pass empty string in lieu of I agree that our tests should ideally cover both controlled/uncontrolled. In this particular PR though, I'm also fine with prioritizing landing just the eslintrc update with minimal changes, and defer any large component/test changes to a separate issue so it can be evaluated/prioritized separately. |
|
My gut tells me that we should respect the convention around passing But that raises the question about how to represent correctly an "empty but controlled" value for a given prop: passing an empty string may cut it for props with As discussed, probably the best next move is to open a separate issue where we discuss "controlled" vs "uncontrolled" strategy for the package. |
|
I agree with Marco in that a broader conversation sounds like it's in order before we move forward on this PR - I'll leave it as is for now, and revisit when we have a clearer picture of how we want to proceed with the controlled/uncontrolled distinction at the package level. |
|
Opened #47473 to discuss separately |
|
Edit — I got ahead of myself, it looks like there's more to that conversation. Let's probably wait a little bit more |
|
@chad1008 is this PR still relevant? |
What?
As discussed in #45044, now that
exhaustive-depsis active for the Components package, it makes sense to includeuseUpdateEffectin the check. There may be other custom hooks we want to include here, but they can be addressed in separate PRs as needed.Why?
For the same reason as the overall
exhaustive-depsmigration, keeping hook dependencies clean can help avoid unexpected behavior creeping into components.How?
Fortunately, there only appears to be one component triggering warnings related to
useUpdateEffect, and it'sToggleGroupControl. There are several warnings in a couple of different spots, more details to follow as this PR develops.With just one component in play, I don't think we need a separate followup PR, so I'll manage all of the work here.
Mostly straightforward changes here. Most notable:
previousValueis actually a ref value provided by theusePrevioushook - so changes won't cause re-renders. Normally it wouldn't be a dependency, buteslintcan't tell that it's actually a ref.onChangeto the dep array does cause the effect to fire more often when the function is defined/destructured, but it is not triggering any additional re-renders.groupContextis defined on each render and therefor wouldn't be referentially stable, but wrapping inuseMemoshould resolve that.radioto theas-radio-groupdep array triggers an additional render as the object gets re-declared each time. I didn't see an easily viable solution, so I've ignored this error for now. It's possible there is an easy fix that I've just overlooked due to the TypeScript of it all.Testing Instructions
npx eslint packages/components/src/exhaustive-depsrule, particularly related touseUpdateEffect