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

Add useControlledValue #33039

Merged
merged 4 commits into from
Jul 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/src/utils/hooks/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export { default as useControlledState } from './use-controlled-state';
export { default as useJumpStep } from './use-jump-step';
export { default as useUpdateEffect } from './use-update-effect';
export { useControlledValue } from './use-controlled-value';
101 changes: 101 additions & 0 deletions packages/components/src/utils/hooks/test/use-controlled-value.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/**
* External dependencies
*/
import { fireEvent, render, screen } from '@testing-library/react';

/**
* Internal dependencies
*/
import { useControlledValue } from '../use-controlled-value';

function Input( props ) {
const [ value, setValue ] = useControlledValue( props );
return (
<input
value={ value }
onChange={ ( event ) => setValue( event.target.value ) }
/>
);
}

function getInput() {
return screen.getByRole( 'textbox' );
}

describe( 'useControlledValue', () => {
it( 'should use the default value', () => {
render( <Input defaultValue="WordPress.org" /> );
expect( getInput() ).toHaveValue( 'WordPress.org' );
} );

it( 'should use the default value then switch to the controlled value', () => {
const { rerender } = render( <Input defaultValue="WordPress.org" /> );
expect( getInput() ).toHaveValue( 'WordPress.org' );

rerender(
<Input defaultValue="WordPress.org" value="Code is Poetry" />
);
expect( getInput() ).toHaveValue( 'Code is Poetry' );
} );

it( 'should not call onChange only when there is no value being passed in', () => {
const onChange = jest.fn();
render( <Input defaultValue="WordPress.org" onChange={ onChange } /> );

expect( getInput() ).toHaveValue( 'WordPress.org' );

fireEvent.change( getInput(), { target: { value: 'Code is Poetry' } } );

expect( getInput() ).toHaveValue( 'Code is Poetry' );
expect( onChange ).not.toHaveBeenCalled();
} );

it( 'should call onChange when there is a value passed in', () => {
const onChange = jest.fn();
const { rerender } = render(
<Input
defaultValue="WordPress.org"
value="Code is Poetry"
onChange={ onChange }
/>
);

expect( getInput() ).toHaveValue( 'Code is Poetry' );

fireEvent.change( getInput(), {
target: { value: 'WordPress rocks!' },
} );

rerender(
Copy link
Contributor

@youknowriad youknowriad Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to rerender in order for the onChange function to get called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to rerender for the onChange function to get called. It's triggered by the fireEvent.change call above that. But our onChange mock function is just jest.fn(). It doesn't really update any state as we would usually do in a component. So the rerender is updating the value to reflect the change. We could write that in a better way though.

<Input
defaultValue="WordPress.org"
value="WordPress rocks!"
onChange={ onChange }
/>
);

expect( getInput() ).toHaveValue( 'WordPress rocks!' );
expect( onChange ).toHaveBeenCalledWith( 'WordPress rocks!' );
} );

it( 'should not maintain internal state if no onChange is passed but a value is passed', () => {
const { rerender } = render( <Input value="Code is Poetry" /> );

expect( getInput() ).toHaveValue( 'Code is Poetry' );

// primarily this proves that the hook doesn't break if no onChange is passed but
// value turns into a controlled state, for example if the value needs to be set
// to a constant in certain conditions but no change listening needs to happen
fireEvent.change( getInput(), { target: { value: 'WordPress.org' } } );

// If `value` is passed then we expect the value to be fully controlled
// meaning that the value passed in will always be used even though
// we're managing internal state.
expect( getInput() ).toHaveValue( 'Code is Poetry' );

// Next we un-set the value to uncover the internal state which was still maintained
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a very weird behavior, why keep an internal state?

Copy link
Member

@diegohaz diegohaz Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is usually called "semi-controlled component" or "optionally controlled component". It's basically trying to mimic the behavior of a native input element on React. We can also consider changing the name of the hook to something like useSemiControlledValue for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using it for TextInput and Slider at the very least.

rerender( <Input /> );

expect( getInput() ).toHaveValue( 'WordPress.org' );
} );
} );
34 changes: 34 additions & 0 deletions packages/components/src/utils/hooks/use-controlled-value.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

type Props< T > = {
defaultValue?: T;
value?: T;
onChange?: ( value: T ) => void;
};

/**
* Simplified and improved implementation of useControlledState.
*
* @param props
* @param props.defaultValue
* @param props.value
* @param props.onChange
* @return The controlled value and the value setter.
*/
export function useControlledValue< T >( {
defaultValue,
onChange,
value: valueProp,
}: Props< T > ): [ T | undefined, ( value: T ) => void ] {
const hasValue = typeof valueProp !== 'undefined';
const initialValue = hasValue ? valueProp : defaultValue;
const [ state, setState ] = useState( initialValue );
const value = hasValue ? valueProp : state;
const setValue =
hasValue && typeof onChange === 'function' ? onChange : setState;

return [ value, setValue ];
}