airbnb
[Suggestion] Remove setting `value: ''` for all input fields
Not sure if this is intentional, but I see here partitionFieldProps sets value = '' for all form fields every time they render:
https://github.com/airbnb/lunar/blob/e37800438b2007ba1ac3821b7edf7713c2970856/packages/core/src/components/FormField/partitionFieldProps.ts#L68-L71
This basically makes an input unusable without a state bind to input.value. Even the most basic input example in the Storybook does not work (users cannot type anything).
It's generally a bad practice to write what users typed right back into an input. It triggers an unnecessary re-render, increases the risk of circular callbacks and greatly increases code complexity. For example, a PR in Bighead had to introduced a new state just to use a debounced onChange handler.
Code as simple as
export default function DebouncedInput(props: DebouncedInputProps) {
const { onChange, debounceTime = 150, ...restProps } = props;
const handleChange = debounce(onChange, debounceTime);
return <Input {...restProps} onChange={handleChange} />;
};
had to change to
export default function DebouncedInput(props: DebouncedInputProps) {
const { onChange, debounceTime = 150, ...restProps } = props;
const [value, setValue] = useState(restProps.value);
const handleChange = debounce(onChange, debounceTime);
return <Input {...restProps} value={value} onChange={(
newValue: string,
event: React.ChangeEvent<HTMLInputElement>
) => {
setValue(newValue);
handleChange(newValue, event);
}} />;
}
And most importantly, manually setting user input as users type has the side effect of interfering with other native browser behaviors, breaking things like autocomplete and foreign language input (I've seen such bugs many many times).
In case a clean input is desired, developers should provide value="" themselves.
The fix is as simple as remove value: ''. If this change would break current developer expectations or other features, it's the best to at least provide an option to disable this behavior.
It's generally a bad practice to write what users typed right back into an input.
Do you mean controlled components are bad in general?
Normally, in the Bighead example, the parent component DeployModal would pass it's own state value search down to the Input field. In this case, because it's Debounced, we want
- Debounced state value in DeployModal
- Instant state value in Input
But that seems like a very specific case right? Outside of debouncing, it's hard to think of examples where an uncontrolled component would be preferable. I'm sure they exist, but this seems like an exception, and not a general rule.
Do you mean controlled components are bad in general?
I actually wasn't aware of controlled components. Thanks for calling it out. My comments mostly came from my experience pre-React era. There were just so many nasty bugs I have dealt with that originated from setting input values as users type.
I agree controlled components can be helpful for most cases, but IMO developers should still have the option to not use it. For simpler form fields, there might not be even a need to use states. Setting value='' took that option away.
Btw, sorry if I sounded more forceful than I actually was. This just reminded me of some very awful bugs.
Haha, no problem.
At least for DX, our UIs almost always adapt to user input in some way, as opposed to having uncontrolled input that we only read on submit. For example:
- Live search/filtering
- Autocomplete
- Field validation
Aside from debouncing, I really struggle to think of a good use case.
FWIW, other component libraries do something similar: https://github.com/ant-design/ant-design/blob/master/components/input/Input.tsx
One downside is that not having a default value allows users to change between a Controlled and Uncontrolled component. I.e. Value starts out undefined, but is later set by the parent component, resulting in:
Warning: Input is changing an uncontrolled input of type text to be controlled. Input elements should not switch from uncontrolled to controlled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component
Since React generates an error anyway, I'm not super concerned about this. I don't have a strong opinion either way.
For debouncing, it's rarely done in handlers. Debouncing is done on API calls, and those are usually throttled instead, outside of the presentational component. It's app logic. I disagree with removing it. The form inputs from the form package handle default values, setting the values, and the flexibility to change them with callbacks like onBatchChange.
@schillerk Ant design does not reset value to '' every time the component rerenders.
See https://codesandbox.io/s/antd-input-vs-lunar-input-oo7tn
IMO this value always resets to '' behavior would be a surprise to most developers. setState should not change where the state is not being used.
If we really want to enforce controlled components and keep this reset, maybe make value a required prop so future developers can feel less surprised.
All form components in core are controlled components, and this is the pattern in which controlled works. Resetting value to an empty string isn't necessarily correct, as the controlled components require the consumer to pass a value in to work. The empty string is just the default state.
Removing that empty stirng is a breaking change, and we just released v3, so this wont happen anytime soon. However, we can probably control this through a prop (like uncontrolled), coupled with partitionFieldProps.
