InputControl: Fix acceptance of falsy values in controlled updates
#42484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +48 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Jul 18, 2022
There was a problem hiding this comment.
Thanks for the quick turnaround and fix @stokesman 🚀
✅ Updated unit test fails as expected on initial commit on the branch
✅ Could still replicate the issue in both Storybook & editors
✅ After checking out latest on PR branch the unit tests pass
✅ Fix resolves the original issue in the editor & storybook.
✅ Components are still functioning in Storybook
| Before | After |
|---|---|
Screen.Recording.2022-07-18.at.10.26.09.am.mp4 |
Screen.Recording.2022-07-18.at.10.45.56.am.mp4 |
LGTM!
ciampo
added
[Type] Bug
|
Would it make sense to add a unit test to avoid future regressions? |
|
That was why I tweaked the test in eb9e1bc. I think it suffices but let me know if you see a fault with that. |
ciampo
reviewed
Jul 19, 2022
There was a problem hiding this comment.
Thank you for the reply — I had missed that small change in my first pass of this PR.
With regards to the changes in this PR, it would be good if we could find an approach that preserves the reducer's "purity" (i.e. accepting action objects).
Since you mentioned this snippet related to the dispatch RESET action
gutenberg/packages/components/src/input-control/reducer/reducer.ts
Lines 98 to 101 in 875a204
Wouldn't be possible to use the null coalescing operator ?? to let falsy values through?
nextState.value = action.payload.value ?? state.initialValue;
That would work for empty strings or
We can add another action (we used to have |
That would my preferred solution — if the
The advantage that I see is that we would keep |
Same. If there are no significant tradeoffs, I would prefer an explicit action type. At least as a casual code reader, I would expect there to always be a |
stokesman
force-pushed
the
fix/reset-of-input-control-based-components
branch
from
5dd5fd6 to
633709a
Compare
stokesman
changed the title
InputControl-based componentsInputControl: Fix acceptance of falsy values in controlled updates
8 tasks
ciampo
reviewed
Jul 21, 2022
| @@ -19,7 +19,7 @@ export interface InputState { | |||
| value?: string; | |||
| } | |||
|
|
|||
| export type StateReducer = Reducer< InputState, InputAction >; | |||
| export type StateReducer = Reducer< InputState, InputAction | ControlAction >; | |||
There was a problem hiding this comment.
Shouldn't the ControlAction be part of the InputAction type ?
There was a problem hiding this comment.
I can see why you'd ask. Looking at this more closely, given we are okay with the way I've wired up CONTROL to be exclusively handled by the base reducer, then I'll say no, and yet it's still not quite right as is. The extending reducers will never receive CONTROL actions so StateReducer would be too loose a type for those.
I'm thinking we should we keep ControlAction out of both InputAction and StateReducer then make only the base reducer typed to accept it.
diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts
index 3f3302608d..a5f80026a6 100644
--- a/packages/components/src/input-control/reducer/reducer.ts
+++ b/packages/components/src/input-control/reducer/reducer.ts
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
-import type { SyntheticEvent, ChangeEvent, PointerEvent } from 'react';
+import type { SyntheticEvent, ChangeEvent, PointerEvent, Reducer } from 'react';
/**
* WordPress dependencies
@@ -50,7 +50,7 @@ function mergeInitialState(
*/
function inputControlStateReducer(
composedStateReducers: StateReducer
-): StateReducer {
+): Reducer< InputState, actions.InputAction | actions.ControlAction > {
return ( state, action ) => {
const nextState = { ...state };
@@ -151,7 +151,7 @@ export function useInputControlStateReducer(
initialState: Partial< InputState > = initialInputControlState,
onChangeHandler: InputChangeCallback
) {
- const [ state, dispatch ] = useReducer< StateReducer >(
+ const [ state, dispatch ] = useReducer(
inputControlStateReducer( stateReducer ),
mergeInitialState( initialState )
);
diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts
index 53a49aa765..2ca9edaa27 100644
--- a/packages/components/src/input-control/reducer/state.ts
+++ b/packages/components/src/input-control/reducer/state.ts
@@ -6,7 +6,7 @@ import type { Reducer, SyntheticEvent } from 'react';
/**
* Internal dependencies
*/
-import type { ControlAction, InputAction } from './actions';
+import type { InputAction } from './actions';
export interface InputState {
_event?: SyntheticEvent;
@@ -19,7 +19,7 @@ export interface InputState {
value?: string;
}
-export type StateReducer = Reducer< InputState, InputAction | ControlAction >;
+export type StateReducer = Reducer< InputState, InputAction >;
export const initialStateReducer: StateReducer = ( state: InputState ) => state;
There was a problem hiding this comment.
The approach that you propose sounds reasonable — maybe we could experiment with generics in the StateReducer type, so that we keep all type defs in the same file?
Something like this
diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts
index 3f3302608d..c41ae85775 100644
--- a/packages/components/src/input-control/reducer/reducer.ts
+++ b/packages/components/src/input-control/reducer/reducer.ts
@@ -50,7 +50,7 @@ function mergeInitialState(
*/
function inputControlStateReducer(
composedStateReducers: StateReducer
-): StateReducer {
+): StateReducer< actions.ControlAction > {
return ( state, action ) => {
const nextState = { ...state };
@@ -151,7 +151,7 @@ export function useInputControlStateReducer(
initialState: Partial< InputState > = initialInputControlState,
onChangeHandler: InputChangeCallback
) {
- const [ state, dispatch ] = useReducer< StateReducer >(
+ const [ state, dispatch ] = useReducer(
inputControlStateReducer( stateReducer ),
mergeInitialState( initialState )
);
diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts
index 53a49aa765..839ac51383 100644
--- a/packages/components/src/input-control/reducer/state.ts
+++ b/packages/components/src/input-control/reducer/state.ts
@@ -6,7 +6,7 @@ import type { Reducer, SyntheticEvent } from 'react';
/**
* Internal dependencies
*/
-import type { ControlAction, InputAction } from './actions';
+import type { InputAction } from './actions';
export interface InputState {
_event?: SyntheticEvent;
@@ -19,7 +19,10 @@ export interface InputState {
value?: string;
}
-export type StateReducer = Reducer< InputState, InputAction | ControlAction >;
+export type StateReducer< TAdditionalAction = {} > = Reducer<
+ InputState,
+ InputAction | TAdditionalAction
+>;
export const initialStateReducer: StateReducer = ( state: InputState ) => state;
We should probably also add a comment about inputControlStateReducer being the only reducer exposing / handling the CONTROL action — which means that it's an action that doesn't get exposed to external reducer middleware, meant to be used only internally. Maybe in the inputControlStateReducer's JSDocs ?
There was a problem hiding this comment.
Done in 9aef4eb though it required a touch of the Action type as well. At first I didn't favor the idea generalizing the StateReducer type for a singular exception to its shape but given that we're interested in refactoring to a hook+component combo #41264 (comment) there will be opportunity make more use of the generic.
There was a problem hiding this comment.
…add a comment about inputControlStateReducer being the only reducer exposing / handling the CONTROL action
Done in f394357
|
Feedback is hopefully addressed satisfactorily. In case not, I've left the rebase/conflict pending for the moment. |
ciampo
reviewed
Jul 28, 2022
There was a problem hiding this comment.
Feedback is hopefully addressed satisfactorily. In case not, I've left the rebase/conflict pending for the moment.
I left one last comment, but the changes required should not add runtime changes to this PR.
When that's addressed, we can rebase and give this PR a final check before merging :)
Comment on lines
22
to
27
| export type StateReducer< SpecializedAction = Action > = Reducer< | ||
| InputState, | ||
| InputAction | SpecializedAction | ||
| >; |
There was a problem hiding this comment.
In the current state of this PR, the StateReducer type would accept any action-like object (this can be verified by inspecting action.type and seeing what possible values are attributed to it by the TypeScript engine).
Instead, what we'd like to achieve is:
- When
SpecializedActionis not defined,StateReducershould only allow actions from theInputActionstype - When
SpecializedActionis defined,StateReducershould only allow actions from either theInputActionstype, or theSpecializedActiontype (although we need to make sure thatSpecializedActionis a validAction).
I played around with code a bit and I think this should be a good solution:
Suggested change
| export type StateReducer< SpecializedAction = Action > = Reducer< | |
| InputState, | |
| InputAction | SpecializedAction | |
| >; | |
| export type StateReducer< SpecializedAction = {} > = Reducer< | |
| InputState, | |
| SpecializedAction extends Action | |
| ? InputAction | SpecializedAction | |
| : InputAction | |
| >; |
There was a problem hiding this comment.
Very neat Marco! Applied in 30d7c31. I don't recall having seen such logic in typing. TIL 🙌
There was a problem hiding this comment.
Thank you! I learnt this technique by reading the definition of the WordPressComponentProps type
gutenberg/packages/components/src/ui/context/wordpress-component.ts
Lines 19 to 24 in 33d84b0
Co-Authored-By: Marco Ciampini <1083581+ciampo@users.noreply.github.com>
stokesman
force-pushed
the
fix/reset-of-input-control-based-components
branch
from
3b54cff to
30d7c31
Compare
ciampo
approved these changes
Jul 29, 2022
|
Thanks for testing and reviewing Aaron and Marco 🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Makes
InputControlhandling of updates from props distinct from any other action.Why?
To fix #42455. Presently the
RESETaction is used to handle updates from props and that favors a fallback value over a falsy one:gutenberg/packages/components/src/input-control/reducer/reducer.ts
Lines 98 to 101 in 875a204
That means that that falsy values coming from props will not be used which is the cause of the issue.
How?
Testing Instructions
npm test-unit input-controlto verify the changed test failsgit switch -