Add useControlledValue #33039
Merged
Add useControlledValue #33039
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: 0 B Total Size: 1.05 MB ℹ️ View Unchanged
|
|
I believe we don't need Here's the same test cases, but re-written with just /**
* 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(
<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
rerender( <Input /> );
expect( getInput() ).toHaveValue( 'WordPress.org' );
} );
} ); |
|
@diegohaz Thanks so much for the component version of those tests. What would you say are the advantages to testing the hook that way rather than using |
To me, |
|
Makes sense to me, thanks for the response! |
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
added
[Feature] Component System
youknowriad
reviewed
Jun 29, 2021
| target: { value: 'WordPress rocks!' }, | ||
| } ); | ||
|
|
||
| rerender( |
There was a problem hiding this comment.
Do we need to rerender in order for the onChange function to get called?
There was a problem hiding this comment.
I believe so.
There was a problem hiding this comment.
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.
youknowriad
reviewed
Jun 29, 2021
| // 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 |
There was a problem hiding this comment.
That is a very weird behavior, why keep an internal state?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We're using it for TextInput and Slider at the very least.
youknowriad
reviewed
Jun 29, 2021
| onChange, | ||
| value: valueProp, | ||
| }: Props< T > ): [ T | undefined, ( value: T ) => void ] { | ||
| const hasValue = ! isNil( valueProp ); |
There was a problem hiding this comment.
why check "nil" and not "undefined". For me "null" is a valid value meaning there's actually a value equal to "null"
There was a problem hiding this comment.
I tend to agree with Riad here but just wanted to bring up that if we use the idiomatic value != null we essentially achieve the same that isNil() does, but avoid unnecessary Lodash usage.
There was a problem hiding this comment.
null isn't a valid value for any controllable element as far as I know. Why should we allow it here?
You'll receive this warning if you try to pass null to an input for example:
"Warning: `value` prop on `input` should not be null. Consider using an empty string to clear the component or `undefined` for uncontrolled components.",
There was a problem hiding this comment.
null isn't a valid value for any controllable element as far as I know. Why should we allow it here?
I think it depends of what you consider a controllable element. If we restrict the definition to DOM element, yes, it's not a valid value but AFAIK, this hook is supposed to be used to make any component controlled? and for instance value=null seems value in a Select component that allows to select an object among a list of objects or none.
There was a problem hiding this comment.
Yes any controllable element, but I hadn't considered anything other inputs as controllable 🤔
There was a problem hiding this comment.
null actually isn't a valid value prop for an option element, if you pass null the value of the select wrapping it will be the text inside the option.
There was a problem hiding this comment.
I was not talking about the html select or option, more about a custom abstraction on top of that, that allows you to pick say a user object in a list of users. And if you want to make that component support both controlled and uncontrolled behavior, you could decide to wrap it with the hook from this PR. (Unless I misunderstood the purpose of that hook :) )
There was a problem hiding this comment.
Gotcha, I can't imagine a custom Select component that doesn't just run on select and option under the hood though... that seems like it would be a lot of over-engineering, right?
I'm happy to change the conditional if you feel strongly about it but testing it will be interesting 🙂
There was a problem hiding this comment.
Gotcha, I can't imagine a custom Select component that doesn't just run on select and option under the hood though... that seems like it would be a lot of over-engineering, right?
I'm not saying it doesn't run the select under the hood. Let me try to clarify here. Imagine we have a controlled Select component that has the following API:
const someArray = [ { id: 'id', label: 'something' } ];
const [ value, setValue ] = useState( null );
<Select value={ value } options={ someArray } onChange={ onChange } />
it allows you to select an object in a list or no object if the value is null, only supports a controlled mode though. (Notice that I didn't mention how the Select is implemented internally but it's most likely using select and options)
Now you want to make this component work in both controlled/uncontrolled mode, If I understand the proposed hook here properly, I should be able to do this:
const MyControlledUncontrolledSelect = ( props ) => {
const [ value, onChange ] = useControlledValue( props );
return <Select value={value} onChange={onChange} />;
}
this won't work properly now because when passing value={ null } the hook will consider the component as "uncontrolled" and never call onChange
I don't feel strongly here personally but I feel that making the check !==undefined is a more generic behavior.
There was a problem hiding this comment.
Aha, thanks for the explanation! I'll make the change 👍
youknowriad
approved these changes
Jun 29, 2021
sarayourfriend
force-pushed
the
add/use-controlled-value
branch
from
c14e3b2
to
0e6ce0b
Compare
12 tasks
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.
Description
Adds a
useControlledValuehook used by a couple new components (TextInput and Slider for example).How has this been tested?
Unit tests.
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).