fix: uncontrolled to controlled input warning#1186
Conversation
krisxcrash
requested review from
dmortal,
hukid,
jeffredodd and
serikjensen
as code owners
serikjensen
left a comment
serikjensen
left a comment
There was a problem hiding this comment.
A couple of things on this one
- I think we need to be extremely cautious around changes that impact core components like this. we need to thoroughly test for regressions across our input components. This will include any components that use TextInput and any components that include the Input component
- I think we should mark this as a breaking change and make it known definitively to our adapter customers that text based inputs now have the empty string passed as a default for value and don't get undefined
| it('stays controlled when value is initially undefined and then defined', () => { | ||
| const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) | ||
|
|
||
| const { rerender } = renderWithProviders( | ||
| <Input placeholder="Controlled" value={undefined} onChange={() => {}} />, | ||
| ) | ||
| const input = screen.getByRole('textbox') | ||
| expect(input).toHaveValue('') | ||
|
|
||
| rerender(<Input placeholder="Controlled" value="hello" onChange={() => {}} />) | ||
| expect(screen.getByRole('textbox')).toHaveValue('hello') | ||
|
|
||
| const errorCalls = errorSpy.mock.calls.filter( | ||
| call => | ||
| typeof call[0] === 'string' && | ||
| call[0].includes('uncontrolled') && | ||
| call[0].includes('controlled'), | ||
| ) | ||
| expect(errorCalls).toHaveLength(0) | ||
| errorSpy.mockRestore() | ||
| }) | ||
|
|
There was a problem hiding this comment.
I think we can probably omit a test for this. we're already verifying that the input behaves as expected with the rest of our suite. there's a lot of errors we could test for the absence of but i think it's better to verify functionality than testing that things are not happening
| ...otherProps | ||
| } = resolvedProps | ||
|
|
||
| const ariaInputProps = 'value' in rawProps ? { ...otherProps, value: value ?? '' } : otherProps |
There was a problem hiding this comment.
I'm confused on this line, why is it checking rawProps? we should be able to just look at the destructured value from resolvedProps to determine if it is defined?
If we're just assigning a default here you should be able to do that when you destructure the resolved props? something like
value = '',
...otherProps
} = resolvedProps
and then you can pass value directly to the aria input props. that might be a bit more clear
| ...otherProps | ||
| } = resolvedProps | ||
|
|
||
| const isControlled = 'value' in rawProps |
There was a problem hiding this comment.
I'm still a little turned around on this one, maybe we could do a quick chat on it? 😅
There was a problem hiding this comment.
Same, should we be instead fixing this at the use-case? Seems like a defaultValues gap perhaps?
3 participants
Summary
Fixes the React warning: "A component is changing an uncontrolled input to be controlled" when
Input(orTextInput) receives avalueprop that is initiallyundefinedand later becomes a defined string.Changes
Input.tsx– When the caller passes avalueprop, the underlying input now always receives a defined value (value ?? ''). Ifvalueis not passed at all, the input remains uncontrolled. This keeps the input consistently controlled whenevervalueis in props and prevents React from seeing a switch from undefined to defined.TextInput.tsx– Passesvalue={value ?? ''}toInputsoundefinednever reachesInputfrom this path (defense-in-depth).Input.test.tsx– Adds a test that rendersInputwithvalue={undefined}then rerenders withvalue="hello", and asserts the input stays in sync and that no uncontrolled/controlled warning is logged toconsole.error.Backwards compatibility
valueprop) is unchanged.valueis unchanged.value={undefined}), which now renders as an empty controlled input and no longer triggers the warning.How to verify
Run Input tests:
npm run test -- --run src/components/Common/UI/Input/Input.test.tsx