Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InputControl: Fix acceptance of falsy values in controlled updates #42484

Merged
merged 11 commits into from
Jul 29, 2022
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Bug Fix

- `BorderControl`: Ensure box-sizing is reset for the control ([#42754](https://github.com/WordPress/gutenberg/pull/42754)).
- `InputControl`: Fix acceptance of falsy values in controlled updates ([#42484](https://github.com/WordPress/gutenberg/pull/42484/)).

### Enhancements

Expand Down
4 changes: 3 additions & 1 deletion packages/components/src/input-control/reducer/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -23,7 +24,7 @@ interface EventPayload {
event?: SyntheticEvent;
}

interface Action< Type, ExtraPayload = {} > {
export interface Action< Type = string, ExtraPayload = {} > {
type: Type;
payload: EventPayload & ExtraPayload;
}
Expand All @@ -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 >;
Expand Down
37 changes: 27 additions & 10 deletions packages/components/src/input-control/reducer/reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,32 @@ 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(
composedStateReducers: StateReducer
): StateReducer {
): StateReducer< actions.ControlAction > {
return ( state, action ) => {
const nextState = { ...state };

switch ( action.type ) {
/*
* Controlled updates
*/
case actions.CONTROL:
nextState.value = action.payload.value;
nextState.isDirty = false;
nextState._event = undefined;
// Returns immediately to avoid invoking additional reducers.
return nextState;

/**
* Keyboard events
*/
Expand Down Expand Up @@ -140,7 +150,7 @@ export function useInputControlStateReducer(
initialState: Partial< InputState > = initialInputControlState,
onChangeHandler: InputChangeCallback
) {
const [ state, dispatch ] = useReducer< StateReducer >(
const [ state, dispatch ] = useReducer(
inputControlStateReducer( stateReducer ),
mergeInitialState( initialState )
);
Expand Down Expand Up @@ -188,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 &&
Expand All @@ -205,14 +220,16 @@ export function useInputControlStateReducer(
} );
}
}, [ state.value, state.isDirty ] );

// Updates the state from props.
useLayoutEffect( () => {
if (
initialState.value !== currentState.current.value &&
! currentState.current.isDirty
) {
dispatch( {
type: actions.RESET,
payload: { value: initialState.value },
type: actions.CONTROL,
payload: { value: initialState.value ?? '' },
ciampo marked this conversation as resolved.
Show resolved Hide resolved
} );
}
}, [ initialState.value ] );
Expand Down
9 changes: 7 additions & 2 deletions packages/components/src/input-control/reducer/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { Reducer, SyntheticEvent } from 'react';
/**
* Internal dependencies
*/
import type { InputAction } from './actions';
import type { Action, InputAction } from './actions';

export interface InputState {
_event?: SyntheticEvent;
Expand All @@ -19,7 +19,12 @@ export interface InputState {
value?: string;
}

export type StateReducer = Reducer< InputState, InputAction >;
export type StateReducer< SpecializedAction = {} > = Reducer<
InputState,
SpecializedAction extends Action
? InputAction | SpecializedAction
: InputAction
>;

export const initialStateReducer: StateReducer = ( state: InputState ) => state;

Expand Down
24 changes: 20 additions & 4 deletions packages/components/src/input-control/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,34 +85,50 @@ 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 ) => {
setState( value );
spy( value );
};
const onKeyDown = ( { key } ) => {
if ( key === 'Escape' ) setState( 'three' );
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 (
<InputControl
value={ state }
onChange={ onChange }
onKeyDown={ onKeyDown }
onKeyUp={ onKeyUp }
/>
);
};
render( <Example /> );
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( '' );

expect( input ).toHaveValue( 'three' );
// 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
* parent component explicitly passed a (new value) change down to
Expand Down