mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.76k stars 32.24k forks source link

[Autocomplete] Warn when mixing uncontrolled and controlled API #18173

Closed RareSecond closed 4 years ago

RareSecond commented 4 years ago

Current Behavior 😯

When passing in a value of undefined and then changing the value to a correct value, stops the AutoComplete from re-rendering.

Expected Behavior 🤔

At least something to happen. Now you're just stuck trying to figure out what's happening. Either default to an empty array, of have something logged in console to warn about it (e.g. like React does when changing from uncontrolled to controlled).

Steps to Reproduce 🕹

https://codesandbox.io/embed/material-demo-dj0w5

Steps:

  1. Check the console. See that the value is undefined.
  2. Click the button at the top.
  3. See that the value is now an array with two elements.
  4. AutoComplete is not showing the new values.
  5. If you change the initial useState() value to useState([]) everything works as expected.

Context 🔦

Our values are fetched from an api. So initially the value is not defined. We could set our initial state to an empty array, but this is conflicting, because that way you don't know when the api call has completed (as an empty array could also be returned by the api call).

Your Environment 🌎

Tech Version
Material-UI latest
React latest
Browser chrome
TypeScript no
oliviertassinari commented 4 years ago

@JDansercoer Oh yes, thank you for the report! I had forgotten to add the warning for this one. If you want to contribute, you can go ahead :)

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index 1d947d6ff..38635d673 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -164,6 +164,25 @@ export default function useAutocomplete(props) {
   });
   const value = isControlled ? valueProp : valueState;

+  if (process.env.NODE_ENV !== 'production') {
+    // eslint-disable-next-line react-hooks/rules-of-hooks
+    React.useEffect(() => {
+      if (isControlled !== (valueProp != null)) {
+        console.error(
+          [
+            `Material-UI: A component is changing ${
+              isControlled ? 'a ' : 'an un'
+            }controlled useAutocomplete to be ${isControlled ? 'un' : ''}controlled.`,
+            'Elements should not switch from uncontrolled to controlled (or vice versa).',
+            'Decide between using a controlled or uncontrolled useAutocomplete ' +
+              'element for the lifetime of the component.',
+            'More info: https://fb.me/react-controlled-components',
+          ].join('\n'),
+        );
+      }
+    }, [valueProp, isControlled]);
+  }
+
   const [inputValue, setInputValue] = React.useState('');
   const [focused, setFocused] = React.useState(false);
RareSecond commented 4 years ago

Before starting on a PR: is it enough to just document this with a warning? Because React also throws warnings when changing from uncontrolled to controlled, but the core functionality still works. However, with the Autocomplete component, the component does not work.

oliviertassinari commented 4 years ago

@JDansercoer We already have 5 occurrences of this exact logic in the codebase. It does warn in both directions.

oliviertassinari commented 4 years ago

but the core functionality still works

I would challenge this affirmation.

RareSecond commented 4 years ago

I would challenge this affirmation.

What exactly do you mean by this?

oliviertassinari commented 4 years ago

You are right it does work with React + <input>: https://codesandbox.io/s/material-demo-wocx2.

It doesn't work with Material-UI. It's a simplification. Should we change it? At the very least, a new warning would be a great first step (use null instead of undefined). I think that we can consider the non-docs approach later on if we get more user feedback.

goodwin64 commented 4 years ago

For those who also faced this issue there is a simple workaround: use null value instead of undefined:

export default function Playground() {
  const defaultProps = {
    options: top100Films,
    getOptionLabel: (option: FilmOptionType) => option.title
  };

  // doesn't work
  // const [value, setValue] = React.useState();

  // works fine
  const [value, setValue] = React.useState(null);

  return (
    <div style={{ width: 300 }}>
       <Autocomplete
        {...defaultProps}
        value={value}
        onChange={(event, newValue) => {
          setValue(newValue);
        }}
        renderInput={params => (
          <TextField {...params} label="controlled" margin="normal" fullWidth />
        )}
      />
      <Autocomplete
        {...defaultProps}
        value={value}
        onChange={(event, newValue) => {
          setValue(newValue);
        }}
        renderInput={params => (
          <TextField {...params} label="controlled2" fullWidth />
        )}
      />
    </div>
  );
}
oliviertassinari commented 4 years ago

Does anyone want to submit a pull request for this problem, based on https://github.com/mui-org/material-ui/issues/18173#issuecomment-549312720?

divrt-srinath commented 4 years ago

I am not using useState, with the below code its not working. What am I doing wrong here?

<Autocomplete
     value={this.state.autoC}
     id="combo-box-demo"
     options={[
            { title: 'red', year: 1994 },
            { title: 'blue', year: 1972 }]}
      getOptionLabel={option => option.title}
      onChange={(evt,value)=>{
            this.setState({
              autoC:value
            })
          }}
       renderInput={params => (
            <TextField {...params} label="Combo box" variant="outlined" fullWidth />
          )}
 />

Edit: I just copied the example from the site, it does not display value

yuechen commented 4 years ago

I actually don't think this console.error is appropriate. It can be totally expected behavior to change the variable passed into defaultValue without expecting the value in the Autocomplete to change. Material UI should not specify this for the developer, and should definitely not throw an error for this (as is currently the behavior). Could be more acceptable as a warning.

oliviertassinari commented 4 years ago

Could be more acceptable as a warning.

@yuechen This resonates with https://github.com/mui-org/material-ui/issues/15343#issuecomment-538759792 about either we should use console.error or console.warn.

In our case, we force developers to be strict about how they handle default values vs. values.

RmStorm commented 4 years ago

I was running into this exact same problem, where I fetch the valid values for a state from an API and so I have to instantiate the state in some empty manner. Basically {} and [] and null work but undefined doesn't. This was a little surprising to me and also not obvious from the documentation linked in the warning.

I expected that since I am explicitly setting the value using value={this.state.myValue} and I instantiate myValue as undefined it will be a controlled component. Maybe this peculiarity should actually be in the warning. Since I assumed I had made a controlled component but by using undefined I actually hadn't.

But the warning is used more general then this specific case so that doesn't seem the place to fix it. It might be best if undefined would be considered a valid default value for a controlled component. But honestly, I have just started learning React so I'm way out of my depth. The current warning is definitely better then no warning but when new users run into it now, it's probably still not clear why the component is switching from uncontrolled to controlled. @oliviertassinari thoughts?

oliviertassinari commented 4 years ago

@RmStorm Thank you for taking the time to write in details your experience. I fear it's expected, however, your mention to the link not helping can be leveraged. I have updated https://github.com/mui-org/material-ui/issues/19423#issuecomment-589862301 to have a mention about it. Thanks (also, feel free to work on it if you want to help us improve the library :)).

Alarees commented 4 years ago

When passing in a value of undefined and then changing the value to a correct value, stops the AutoComplete from re-rendering.

thanks @JDansercoer , this save my day now i change my code to value={formState.attributes[item] || ""}

tsognong commented 3 years ago

When passing in a value of undefined and then changing the value to a correct value, stops the AutoComplete from re-rendering.

thanks @JDansercoer , this save my day now i change my code to value={formState.attributes[item] || ""}

Thanks @Alarees this also save my days.

vitorregisrr commented 7 months ago

For those who also faced this issue there is a simple workaround: use null value instead of undefined:

export default function Playground() {
  const defaultProps = {
    options: top100Films,
    getOptionLabel: (option: FilmOptionType) => option.title
  };

  // doesn't work
  // const [value, setValue] = React.useState();

  // works fine
  const [value, setValue] = React.useState(null);

  return (
    <div style={{ width: 300 }}>
       <Autocomplete
        {...defaultProps}
        value={value}
        onChange={(event, newValue) => {
          setValue(newValue);
        }}
        renderInput={params => (
          <TextField {...params} label="controlled" margin="normal" fullWidth />
        )}
      />
      <Autocomplete
        {...defaultProps}
        value={value}
        onChange={(event, newValue) => {
          setValue(newValue);
        }}
        renderInput={params => (
          <TextField {...params} label="controlled2" fullWidth />
        )}
      />
    </div>
  );
}

Wtf. I lost hours to find that. Don't make sense MUI works this way, but ok. Thank you so much for this explanation. <3 I changed my: <Autocomplete value={ modelos_saida.find( ({ id }) => id == formik.values.modelo_saida_bem ) }

For: <Autocomplete value={ modelos_saida.find( ({ id }) => id == formik.values.modelo_saida_bem ) || null }

And now it's getting the initial value from my state.

For anyone trying to debug or solve that and want a explanation:

I'm guessing that if the value starts as undefined in the first render, the component not re-render the value by changing the state programmatically (like the state updated when a fetch is completed). It will only change if the user select a option manually by UI.

If the value start as null it re-render correctly when the state changes.