Open mikelambert opened 7 years ago
Hi Mike! In the very beginning this component was in fact uncontrolled. The main problem we had at that point was that it was in effect impossible to update the component's value
without directly referencing the (undocumented) internal ref
state. We decided to make the component controlled with the logic that if it's controlled, at least you have the option of wrapping it with another component to make it uncontrolled.
That's typically the advice I've given to people who need an uncontrolled component: create another component which stores value
in its own state, then expose defaultValue
and whichever other prop you need to get it bootstrapped.
Now I realise that that's slightly more work, and people are going to end up writing very similar code to achieve this. In my experience I almost always end up writing a bespoke wrapper which takes care of app-specific logic, layout, and styling, so adding the code to handle value
is only 3-4 extra lines.
When it comes to performance: Even if we maintained value
within Autocomplete
, chances are we would have to re-render the entire component whenever value
changes (due to the nature of how the items are filtered and displayed), so I don't know if there would be much of a gain by doing this. I would rather try to see if you can wrap Autocomplete
in PureComponent
to see if you can avoid unnecessary tree creation and diffing.
This feature request keeps coming up every few months, and it's definitely something we have in the back of our minds. It will probably be on the v2 roadmap somewhere, if the grand design allows it.
Very good point about performance, that even in uncontrolled mode, it stills requires a re-render each time the value changes anyway. Doh!
I suspect it will be a bit faster since only renderMenu
needs to be re-rendered, not the <input>
, and not the surrounding React component that might otherwise manage state. And it avoids the need to do additional callbacks. But not too much.
(Naively, my state was stored in a FormComponent, which caused multiple AutoCompletes to needlessly re-renderer. I've since split up my state into multiple components, though PureComponent
would have worked too.)
However, I don't think it's possible to wrap this as an uncontrolled component. Adding that support, would also be a valid solution that would work for me.
To do this, react-autocomplete would need to:
defaultValue
instead of value
to <input .../>
. This is now possible as of a week ago in 1.6
with renderInput
(I didn't realize it at the time I filed this, since I was running 1.5.x
), with this:
renderInput={args => {
const { value, ...otherArgs } = args;
return <input defaultValue={this.props.value} {...otherArgs} />;
}}
props.value
. Replacing such references with an accessor that returns props.value !== undefined ? props.value || this.refs.input.value
would work better. (Or maybe an props.valueAccessorFunction
I can override, that does the same thing?)I have also checked a second iPhone, and noticed that my newer iPhone appears to have serious performance issues overall. And so it might be unfair for my to judge the performance here off my possibly-broken device.
Wether or not the component should be allowed uncontrolled or not aside. The documentation does suggest this is the case because it states that the value attribute is optional.
When I do omit the value attribute te component does not render and throws an error.
It appears this component cannot be used in uncontrolled fashion, and requires that you specify
value=
and control it from outside state. I'm curious if this has to be the case, or if you'd be open to changes to using it in an uncontrolled fashion?Given that the state is also stored inside the form, I'm happy to have my code submit that form, or extract data from that form directly.
In particular, I'm noticing my webapp being really laggy on my mobile device, and am exploring causes. One potential cause being the overhead on each
onChange
event (that causes my form to re-render just to pass down a newvalue=
).I saw this was briefly brought up in https://github.com/reactjs/react-autocomplete/issues/113 , but was then quickly discarded.