From 00108ac5969b74e783caf4a9b3bc9cec88f7c297 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 17 Jul 2022 08:54:22 -0700 Subject: [PATCH 01/10] Update controlled component test to use falsy value --- packages/components/src/input-control/test/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/components/src/input-control/test/index.js b/packages/components/src/input-control/test/index.js index ebdf97ee8d7a3..b7c63c8e26e21 100644 --- a/packages/components/src/input-control/test/index.js +++ b/packages/components/src/input-control/test/index.js @@ -95,7 +95,7 @@ describe( 'InputControl', () => { spy( value ); }; const onKeyDown = ( { key } ) => { - if ( key === 'Escape' ) setState( 'three' ); + if ( key === 'Escape' ) setState( '' ); }; return ( { // Make a controlled update. await user.keyboard( '{Escape}' ); - expect( input ).toHaveValue( 'three' ); + expect( input ).toHaveValue( '' ); /* * onChange called only once. onChange is not called when a * parent component explicitly passed a (new value) change down to From cd2ece58510b933f24c62a89a536f9335fa3a5b2 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sat, 16 Jul 2022 13:44:07 -0700 Subject: [PATCH 02/10] Handle controlled updates specially in `InputControl` --- .../src/input-control/reducer/reducer.ts | 20 +++++++++++++------ .../src/input-control/reducer/state.ts | 4 ++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 9d9d4787ecae0..9195179d194fc 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -13,6 +13,7 @@ import { useReducer, useLayoutEffect, useRef } from '@wordpress/element'; */ import { InputState, + LeadStateReducer, StateReducer, initialInputControlState, initialStateReducer, @@ -50,8 +51,18 @@ function mergeInitialState( */ function inputControlStateReducer( composedStateReducers: StateReducer -): StateReducer { +): LeadStateReducer { return ( state, action ) => { + if ( ! ( 'type' in action ) ) { + // Returns the new state without running other reducers. These are + // controlled updates from props and need no specialization. + return { + ...state, + value: `${ action.value ?? '' }`, + isDirty: false, + _event: undefined, + }; + } const nextState = { ...state }; switch ( action.type ) { @@ -140,7 +151,7 @@ export function useInputControlStateReducer( initialState: Partial< InputState > = initialInputControlState, onChangeHandler: InputChangeCallback ) { - const [ state, dispatch ] = useReducer< StateReducer >( + const [ state, dispatch ] = useReducer< LeadStateReducer >( inputControlStateReducer( stateReducer ), mergeInitialState( initialState ) ); @@ -210,10 +221,7 @@ export function useInputControlStateReducer( initialState.value !== currentState.current.value && ! currentState.current.isDirty ) { - dispatch( { - type: actions.RESET, - payload: { value: initialState.value }, - } ); + dispatch( { value: initialState.value } ); } }, [ initialState.value ] ); diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index 2ca9edaa27b20..4f02461321761 100644 --- a/packages/components/src/input-control/reducer/state.ts +++ b/packages/components/src/input-control/reducer/state.ts @@ -19,6 +19,10 @@ export interface InputState { value?: string; } +export type LeadStateReducer = Reducer< + InputState, + InputAction | Partial< InputState > +>; export type StateReducer = Reducer< InputState, InputAction >; export const initialStateReducer: StateReducer = ( state: InputState ) => state; From a2ee306d4bcf13d427c73c53e4730ca2677c3857 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Sun, 17 Jul 2022 13:10:15 -0700 Subject: [PATCH 03/10] Add changelog entry --- packages/components/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 57bb3e24c4b60..fd76e4a0ea315 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -13,6 +13,7 @@ - `ColorPalette`: Fix background image in RTL mode ([#42510](https://github.com/WordPress/gutenberg/pull/42510)). - `RangeControl`: clamp initialPosition between min and max values ([#42571](https://github.com/WordPress/gutenberg/pull/42571)). - `Tooltip`: avoid unnecessary re-renders of select child elements ([#42483](https://github.com/WordPress/gutenberg/pull/42483)). +- `InputControl`: Fix acceptance of falsy values in controlled updates ([#42484](https://github.com/WordPress/gutenberg/pull/42484/)). ### Enhancements From 1585729fe87b97a6d7748cdaeb6abb59d48ceb08 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 21 Jul 2022 09:01:20 -0700 Subject: [PATCH 04/10] Replace typeless action with new CONTROL action --- .../src/input-control/reducer/actions.ts | 2 ++ .../src/input-control/reducer/reducer.ts | 31 ++++++++++--------- .../src/input-control/reducer/state.ts | 8 ++--- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/packages/components/src/input-control/reducer/actions.ts b/packages/components/src/input-control/reducer/actions.ts index d66eb5778aad8..846421dbc5e8e 100644 --- a/packages/components/src/input-control/reducer/actions.ts +++ b/packages/components/src/input-control/reducer/actions.ts @@ -10,6 +10,7 @@ import type { DragProps } from '../types'; export const CHANGE = 'CHANGE'; export const COMMIT = 'COMMIT'; +export const CONTROL = 'CONTROL'; export const DRAG_END = 'DRAG_END'; export const DRAG_START = 'DRAG_START'; export const DRAG = 'DRAG'; @@ -34,6 +35,7 @@ interface ValuePayload { export type ChangeAction = Action< typeof CHANGE, ValuePayload >; export type CommitAction = Action< typeof COMMIT, ValuePayload >; +export type ControlAction = Action< typeof CONTROL, ValuePayload >; export type PressUpAction = Action< typeof PRESS_UP >; export type PressDownAction = Action< typeof PRESS_DOWN >; export type PressEnterAction = Action< typeof PRESS_ENTER >; diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 9195179d194fc..3f3302608daeb 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -13,7 +13,6 @@ import { useReducer, useLayoutEffect, useRef } from '@wordpress/element'; */ import { InputState, - LeadStateReducer, StateReducer, initialInputControlState, initialStateReducer, @@ -51,21 +50,22 @@ function mergeInitialState( */ function inputControlStateReducer( composedStateReducers: StateReducer -): LeadStateReducer { +): StateReducer { return ( state, action ) => { - if ( ! ( 'type' in action ) ) { - // Returns the new state without running other reducers. These are - // controlled updates from props and need no specialization. - return { - ...state, - value: `${ action.value ?? '' }`, - isDirty: false, - _event: undefined, - }; - } const nextState = { ...state }; switch ( action.type ) { + /* + * Controlled updates + */ + case actions.CONTROL: + nextState.value = `${ action.payload.value }`; + nextState.isDirty = false; + nextState._event = undefined; + // So far there is no need for specialization of this action so + // returning immediately avoids invoking additional reducers. + return nextState; + /** * Keyboard events */ @@ -151,7 +151,7 @@ export function useInputControlStateReducer( initialState: Partial< InputState > = initialInputControlState, onChangeHandler: InputChangeCallback ) { - const [ state, dispatch ] = useReducer< LeadStateReducer >( + const [ state, dispatch ] = useReducer< StateReducer >( inputControlStateReducer( stateReducer ), mergeInitialState( initialState ) ); @@ -221,7 +221,10 @@ export function useInputControlStateReducer( initialState.value !== currentState.current.value && ! currentState.current.isDirty ) { - dispatch( { value: initialState.value } ); + dispatch( { + type: actions.CONTROL, + payload: { value: initialState.value ?? '' }, + } ); } }, [ initialState.value ] ); diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index 4f02461321761..53a49aa765feb 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 { InputAction } from './actions'; +import type { ControlAction, InputAction } from './actions'; export interface InputState { _event?: SyntheticEvent; @@ -19,11 +19,7 @@ export interface InputState { value?: string; } -export type LeadStateReducer = Reducer< - InputState, - InputAction | Partial< InputState > ->; -export type StateReducer = Reducer< InputState, InputAction >; +export type StateReducer = Reducer< InputState, InputAction | ControlAction >; export const initialStateReducer: StateReducer = ( state: InputState ) => state; From b6a48627f3017b4025ba1e3220805cc813babb95 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 21 Jul 2022 09:11:54 -0700 Subject: [PATCH 05/10] Further cover controlled updates in unit test --- .../src/input-control/test/index.js | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/components/src/input-control/test/index.js b/packages/components/src/input-control/test/index.js index b7c63c8e26e21..301d5759c0664 100644 --- a/packages/components/src/input-control/test/index.js +++ b/packages/components/src/input-control/test/index.js @@ -85,9 +85,10 @@ describe( 'InputControl', () => { expect( spy ).toHaveBeenLastCalledWith( 'Hello there' ); } ); - it( 'should work as a controlled component', async () => { + it( 'should work as a controlled component given normal, falsy or nullish values', async () => { const user = setupUser(); const spy = jest.fn(); + const heldKeySet = new Set(); const Example = () => { const [ state, setState ] = useState( 'one' ); const onChange = ( value ) => { @@ -95,13 +96,21 @@ describe( 'InputControl', () => { spy( value ); }; const onKeyDown = ( { key } ) => { - if ( key === 'Escape' ) setState( '' ); + heldKeySet.add( key ); + if ( key === 'Escape' ) { + if ( heldKeySet.has( 'Meta' ) ) setState( 'qux' ); + else if ( heldKeySet.has( 'Alt' ) ) + setState( undefined ); + else setState( '' ); + } }; + const onKeyUp = ( { key } ) => heldKeySet.delete( key ); return ( ); }; @@ -109,9 +118,16 @@ describe( 'InputControl', () => { const input = getInput(); await user.type( input, '2' ); - // Make a controlled update. + // Make a controlled update with a falsy value. await user.keyboard( '{Escape}' ); + expect( input ).toHaveValue( '' ); + + // Make a controlled update with a normal value. + await user.keyboard( '{Meta>}{Escape}{/Meta}' ); + expect( input ).toHaveValue( 'qux' ); + // Make a controlled update with a nullish value. + await user.keyboard( '{Alt>}{Escape}{/Alt}' ); expect( input ).toHaveValue( '' ); /* * onChange called only once. onChange is not called when a From 76de26480f56ee7dc7c1d9d55af348676b82325d Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 27 Jul 2022 08:34:50 -0700 Subject: [PATCH 06/10] Refine typing of base reducer --- packages/components/src/input-control/reducer/actions.ts | 2 +- packages/components/src/input-control/reducer/reducer.ts | 4 ++-- packages/components/src/input-control/reducer/state.ts | 7 +++++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/components/src/input-control/reducer/actions.ts b/packages/components/src/input-control/reducer/actions.ts index 846421dbc5e8e..374d53433d372 100644 --- a/packages/components/src/input-control/reducer/actions.ts +++ b/packages/components/src/input-control/reducer/actions.ts @@ -24,7 +24,7 @@ interface EventPayload { event?: SyntheticEvent; } -interface Action< Type, ExtraPayload = {} > { +export interface Action< Type = string, ExtraPayload = {} > { type: Type; payload: EventPayload & ExtraPayload; } diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 3f3302608daeb..c41ae85775538 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 53a49aa765feb..a26dacdd83f4e 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 { Action, 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< SpecializedAction = Action > = Reducer< + InputState, + InputAction | SpecializedAction +>; export const initialStateReducer: StateReducer = ( state: InputState ) => state; From f930e06bdbdad502d5b9cdb7005c7ac5c4f8b038 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 27 Jul 2022 08:45:57 -0700 Subject: [PATCH 07/10] Revise comment --- .../src/input-control/reducer/reducer.ts | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index c41ae85775538..65672775b62fe 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -39,13 +39,13 @@ function mergeInitialState( } /** - * Creates a reducer that opens the channel for external state subscription - * and modification. + * Creates the base reducer which may be coupled to a specializing reducer. + * As its final step, for all actions other than CONTROL, the base reducer + * passes the state and action on through the specializing reducer. The + * exception for CONTROL actions is because they represent controlled updates + * from props and no case has yet presented for their specialization. * - * This technique uses the "stateReducer" design pattern: - * https://kentcdodds.com/blog/the-state-reducer-pattern/ - * - * @param composedStateReducers A custom reducer that can subscribe and modify state. + * @param composedStateReducers A reducer to specialize state changes. * @return The reducer. */ function inputControlStateReducer( @@ -62,8 +62,7 @@ function inputControlStateReducer( nextState.value = `${ action.payload.value }`; nextState.isDirty = false; nextState._event = undefined; - // So far there is no need for specialization of this action so - // returning immediately avoids invoking additional reducers. + // Returns immediately to avoid invoking additional reducers. return nextState; /** From b9e111c8bd1e492ed1027883a5cb5b8ee47c5ca1 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 27 Jul 2022 08:50:31 -0700 Subject: [PATCH 08/10] Revert runtime string ensurance --- packages/components/src/input-control/reducer/reducer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index 65672775b62fe..b26ccf3a31001 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -59,7 +59,7 @@ function inputControlStateReducer( * Controlled updates */ case actions.CONTROL: - nextState.value = `${ action.payload.value }`; + nextState.value = action.payload.value; nextState.isDirty = false; nextState._event = undefined; // Returns immediately to avoid invoking additional reducers. From 502ad98aee6565d367ab8f15048a88156d9f5f3a Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Wed, 27 Jul 2022 08:51:55 -0700 Subject: [PATCH 09/10] Comment purpose of effects --- packages/components/src/input-control/reducer/reducer.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/components/src/input-control/reducer/reducer.ts b/packages/components/src/input-control/reducer/reducer.ts index b26ccf3a31001..acd95d2664342 100644 --- a/packages/components/src/input-control/reducer/reducer.ts +++ b/packages/components/src/input-control/reducer/reducer.ts @@ -198,10 +198,15 @@ export function useInputControlStateReducer( const currentState = useRef( state ); const refProps = useRef( { value: initialState.value, onChangeHandler } ); + + // Freshens refs to props and state so that subsequent effects have access + // to their latest values without their changes causing effect runs. useLayoutEffect( () => { currentState.current = state; refProps.current = { value: initialState.value, onChangeHandler }; } ); + + // Propagates the latest state through onChange. useLayoutEffect( () => { if ( currentState.current._event !== undefined && @@ -215,6 +220,8 @@ export function useInputControlStateReducer( } ); } }, [ state.value, state.isDirty ] ); + + // Updates the state from props. useLayoutEffect( () => { if ( initialState.value !== currentState.current.value && From 30d7c3190bbdd747a7456ccffefd01c548bc3112 Mon Sep 17 00:00:00 2001 From: Mitchell Austin Date: Thu, 28 Jul 2022 08:54:29 -0700 Subject: [PATCH 10/10] Improve typing of `StateReducer` Co-Authored-By: Marco Ciampini <1083581+ciampo@users.noreply.github.com> --- packages/components/src/input-control/reducer/state.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/components/src/input-control/reducer/state.ts b/packages/components/src/input-control/reducer/state.ts index a26dacdd83f4e..3ebb24669a342 100644 --- a/packages/components/src/input-control/reducer/state.ts +++ b/packages/components/src/input-control/reducer/state.ts @@ -19,9 +19,11 @@ export interface InputState { value?: string; } -export type StateReducer< SpecializedAction = Action > = Reducer< +export type StateReducer< SpecializedAction = {} > = Reducer< InputState, - InputAction | SpecializedAction + SpecializedAction extends Action + ? InputAction | SpecializedAction + : InputAction >; export const initialStateReducer: StateReducer = ( state: InputState ) => state;