-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Autocomplete] Warn when mixing controlled/uncontrolled inputValue states #20403
[Autocomplete] Warn when mixing controlled/uncontrolled inputValue states #20403
Conversation
useAutocomplete hook assumes, that string value is passed in inputValue property. If an undefined value is passed for whatever case, the client will crash due to length checks against the undefined value
Details of bundle changes.Comparing: 861498c...7242344 Details of page changes
|
maybe is better to not crash and throw a big warning? |
Added a warning to be logged when inputValue is defaulted to empty string, would that do? |
@vileppanen Thanks for starting an effort in this direction. Do you have a reproduction of the issue you are facing? In the meantime, I have pushed the pull request in the direction proposed in #19423. We have seen a non-neglieable number of developers that mix the purpose of the value and input value state (your pull request issue description seems to fall into the same trap, but not sure hence why I have asked for a live reproduction). |
I was almost sure, I had the reproduction ready here |
@vileppanen I meant a reproduction where the error originates from within the Autocomplete component. |
Ah yes, it seems to crash in this line https://github.com/mui-org/material-ui/blob/861498c05fc27d794b10803928b8164709772409/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L864 (although I'm not sure if that's the only place for causing troubles, as the inputValue is been referred to in other places too) |
@vileppanen Ok, thanks, it helps a bit. So, the component is originally controlled, then the prop switches to undefined. So it crashes as expected. The pull request introduces a warning to help developers understand the resolution. |
I have the same issue I am using |
Don't recall much of this case, but as I remember, there wasn't an actual "resolution" per se involved, other than enhancing the documentation on how Do you have some repro of your case to look at? |
In freeSolo mode, useAutocomplete hook assumes, that a string value is passed in inputValue property. If an undefined value is passed for whatever reason, the client will crash due to length checks against the undefined value.
Thought that more desirable approach would be to just set the input value as empty string, instead of crashing. In this case, the blame is inherently in the client which is rendering the Autocomplete and misunderstanding the signature of the
onChange
function. However, it also seems fragile that theuseAutocomplete
hook assumes the rendering components to pass in "non-null/defined" values.Steps to reproduce:
freeSolo
prop to AutocompleteinputValue
prop to Autocomplete with value handled in parent componentonChange
prop to Autocomplete with function that will change theinputValue
to be undefinedHere's sandbox for the previous example:
https://codesandbox.io/s/autocompleteselectnpe-5mc47
Also, seems that this issue is loosely related to the same thing: for not being prepared to undefined values?
#20152
Closes #19423
Edit by @oliviertassinari