[Autocomplete] Warn when mixing controlled/uncontrolled inputValue st…

…ates (#20403) * [Autocomplete] fallback to empty string if input value undefined 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 * [Autocomplete] prettier fixes * [Autocomplete] log warning when undefined inputValue * [Autocomplete] add tests for inputValue handling * [Autocomplete] fix test mishap * [Autocomplete] Clarify value and inputValue link Co-authored-by: Olivier Tassinari <[email protected]>
mui · Apr 4, 2020 · 1e8b293 · 1e8b293
1 parent 9f8bf50
commit 1e8b293
Show file tree

Hide file tree

Showing 15 changed files with 51 additions and 37 deletions.
diff --git a/docs/src/pages/components/autocomplete/autocomplete.md b/docs/src/pages/components/autocomplete/autocomplete.md
@@ -32,6 +32,15 @@ Choose one of the 248 countries.
 
 {{"demo": "pages/components/autocomplete/CountrySelect.js"}}
 
+### Controllable states
+
+The component has two states that can be controlled:
+
+1. the "value" state with the `value`/`onChange` props combination.
+2. the "input value" state with the `inputValue`/`onInputChange` props combination.
+
+> ⚠️ These two state are isolated, they should be controlled independently.
+
 ## Free solo
 
 Set `freeSolo` to true so the textbox can contain any arbitrary value. The prop is designed to cover the primary use case of a search box with suggestions, e.g. Google search.

diff --git a/packages/material-ui-lab/src/Pagination/usePagination.js b/packages/material-ui-lab/src/Pagination/usePagination.js
@@ -25,6 +25,7 @@ export default function usePagination(props = {}) {
     controlled: pageProp,
     default: defaultPage,
     name: componentName,
+    state: 'page',
   });
 
   const handleClick = (event, value) => {

diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js
@@ -67,12 +67,14 @@ const TreeView = React.forwardRef(function TreeView(props, ref) {
     controlled: expandedProp,
     default: defaultExpanded,
     name: 'TreeView',
+    state: 'expanded',
   });
 
   const [selected, setSelectedState] = useControlled({
     controlled: selectedProp,
     default: defaultSelected,
     name: 'TreeView',
+    state: 'selected',
   });
 
   /*

diff --git a/packages/material-ui-lab/src/TreeView/TreeView.test.js b/packages/material-ui-lab/src/TreeView/TreeView.test.js
@@ -46,7 +46,7 @@ describe('<TreeView />', () => {
 
       setProps({ expanded: undefined });
       expect(consoleErrorMock.messages()[0]).to.include(
-        'A component is changing a controlled TreeView to be uncontrolled',
+        'Material-UI: a component is changing the controlled expanded state of TreeView to be uncontrolled.',
       );
     });
 
@@ -59,7 +59,7 @@ describe('<TreeView />', () => {
 
       setProps({ selected: undefined });
       expect(consoleErrorMock.messages()[0]).to.include(
-        'A component is changing a controlled TreeView to be uncontrolled',
+        'Material-UI: a component is changing the controlled selected state of TreeView to be uncontrolled.',
       );
     });
   });

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -186,10 +186,12 @@ export default function useAutocomplete(props) {
     default: defaultValue,
     name: componentName,
   });
-
-  const { current: isInputValueControlled } = React.useRef(inputValueProp != null);
-  const [inputValueState, setInputValue] = React.useState('');
-  const inputValue = isInputValueControlled ? inputValueProp : inputValueState;
+  const [inputValue, setInputValue] = useControlled({
+    controlled: inputValueProp,
+    default: '',
+    name: componentName,
+    state: 'inputValue',
+  });
 
   const [focused, setFocused] = React.useState(false);
 

diff --git a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.js
@@ -99,6 +99,7 @@ const ExpansionPanel = React.forwardRef(function ExpansionPanel(props, ref) {
     controlled: expandedProp,
     default: defaultExpanded,
     name: 'ExpansionPanel',
+    state: 'expanded',
   });
 
   const handleChange = React.useCallback(

diff --git a/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js b/packages/material-ui/src/ExpansionPanel/ExpansionPanel.test.js
@@ -1,6 +1,6 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
-import { assert } from 'chai';
+import { assert, expect } from 'chai';
 import { spy } from 'sinon';
 import { createMount, getClasses, findOutermostIntrinsic } from '@material-ui/core/test-utils';
 import describeConformance from '../test-utils/describeConformance';
@@ -193,9 +193,8 @@ describe('<ExpansionPanel />', () => {
       const wrapper = mount(<ExpansionPanel expanded>{minimalChildren}</ExpansionPanel>);
 
       wrapper.setProps({ expanded: undefined });
-      assert.include(
-        consoleErrorMock.messages()[0],
-        'A component is changing a controlled ExpansionPanel to be uncontrolled.',
+      expect(consoleErrorMock.messages()[0]).to.include(
+        'Material-UI: a component is changing the controlled expanded state of ExpansionPanel to be uncontrolled.',
       );
     });
 
@@ -205,7 +204,7 @@ describe('<ExpansionPanel />', () => {
       wrapper.setProps({ expanded: true });
       assert.include(
         consoleErrorMock.messages()[0],
-        'A component is changing an uncontrolled ExpansionPanel to be controlled.',
+        'Material-UI: a component is changing the uncontrolled expanded state of ExpansionPanel to be controlled.',
       );
     });
   });

diff --git a/packages/material-ui/src/RadioGroup/RadioGroup.test.js b/packages/material-ui/src/RadioGroup/RadioGroup.test.js
@@ -348,9 +348,8 @@ describe('<RadioGroup />', () => {
       );
 
       wrapper.setProps({ value: undefined });
-      assert.include(
-        consoleErrorMock.messages()[0],
-        'A component is changing a controlled RadioGroup to be uncontrolled.',
+      expect(consoleErrorMock.messages()[0]).to.include(
+        'Material-UI: a component is changing the controlled value state of RadioGroup to be uncontrolled.',
       );
     });
 
@@ -362,9 +361,8 @@ describe('<RadioGroup />', () => {
       );
 
       wrapper.setProps({ value: 'foo' });
-      assert.include(
-        consoleErrorMock.messages()[0],
-        'A component is changing an uncontrolled RadioGroup to be controlled.',
+      expect(consoleErrorMock.messages()[0]).to.include(
+        'Material-UI: a component is changing the uncontrolled value state of RadioGroup to be controlled.',
       );
     });
   });

diff --git a/packages/material-ui/src/Slider/Slider.test.js b/packages/material-ui/src/Slider/Slider.test.js
@@ -515,7 +515,7 @@ describe('<Slider />', () => {
 
       setProps({ value: undefined });
       expect(consoleErrorMock.messages()[0]).to.include(
-        'A component is changing a controlled Slider to be uncontrolled.',
+        'Material-UI: a component is changing the controlled value state of Slider to be uncontrolled.',
       );
     });
 
@@ -524,7 +524,7 @@ describe('<Slider />', () => {
 
       setProps({ value: [20, 50] });
       expect(consoleErrorMock.messages()[0]).to.include(
-        'A component is changing an uncontrolled Slider to be controlled.',
+        'Material-UI: a component is changing the uncontrolled value state of Slider to be controlled.',
       );
     });
   });

diff --git a/packages/material-ui/src/Tooltip/Tooltip.js b/packages/material-ui/src/Tooltip/Tooltip.js
@@ -215,6 +215,7 @@ const Tooltip = React.forwardRef(function Tooltip(props, ref) {
     controlled: openProp,
     default: false,
     name: 'Tooltip',
+    state: 'open',
   });
   let open = openState;
 

diff --git a/packages/material-ui/src/Tooltip/Tooltip.test.js b/packages/material-ui/src/Tooltip/Tooltip.test.js
@@ -522,9 +522,8 @@ describe('<Tooltip />', () => {
       const wrapper = mount(<Tooltip {...defaultProps} />);
 
       wrapper.setProps({ open: true });
-      assert.include(
-        consoleErrorMock.messages()[0],
-        'A component is changing an uncontrolled Tooltip to be controlled.',
+      expect(consoleErrorMock.messages()[0]).to.include(
+        'Material-UI: a component is changing the uncontrolled open state of Tooltip to be controlled.',
       );
     });
   });

diff --git a/packages/material-ui/src/internal/SwitchBase.js b/packages/material-ui/src/internal/SwitchBase.js
@@ -58,6 +58,7 @@ const SwitchBase = React.forwardRef(function SwitchBase(props, ref) {
     controlled: checkedProp,
     default: Boolean(defaultChecked),
     name: 'SwitchBase',
+    state: 'checked',
   });
 
   const muiFormControl = useFormControl();

diff --git a/packages/material-ui/src/internal/SwitchBase.test.js b/packages/material-ui/src/internal/SwitchBase.test.js
@@ -374,10 +374,10 @@ describe('<SwitchBase />', () => {
         wrapper.setProps({ checked: true });
         expect(consoleErrorMock.callCount()).to.equal(2);
         expect(consoleErrorMock.messages()[0]).to.include(
-          'A component is changing an uncontrolled input of type checkbox to be controlled.',
+          'Warning: A component is changing an uncontrolled input of type checkbox to be controlled.',
         );
         expect(consoleErrorMock.messages()[1]).to.include(
-          'A component is changing an uncontrolled SwitchBase to be controlled.',
+          'Material-UI: a component is changing the uncontrolled checked state of SwitchBase to be controlled.',
         );
       }),
     );
@@ -393,10 +393,10 @@ describe('<SwitchBase />', () => {
         setProps({ checked: undefined });
         expect(consoleErrorMock.callCount()).to.equal(2);
         expect(consoleErrorMock.messages()[0]).to.include(
-          'A component is changing a controlled input of type checkbox to be uncontrolled.',
+          'Warning: A component is changing a controlled input of type checkbox to be uncontrolled.',
         );
         expect(consoleErrorMock.messages()[1]).to.include(
-          'A component is changing a controlled SwitchBase to be uncontrolled.',
+          'Material-UI: a component is changing the controlled checked state of SwitchBase to be uncontrolled.',
         );
       }),
     );

diff --git a/packages/material-ui/src/utils/useControlled.js b/packages/material-ui/src/utils/useControlled.js
@@ -1,7 +1,7 @@
 /* eslint-disable react-hooks/rules-of-hooks, react-hooks/exhaustive-deps */
 import * as React from 'react';
 
-export default function useControlled({ controlled, default: defaultProp, name }) {
+export default function useControlled({ controlled, default: defaultProp, name, state = 'value' }) {
   const { current: isControlled } = React.useRef(controlled !== undefined);
   const [valueState, setValue] = React.useState(defaultProp);
   const value = isControlled ? controlled : valueState;
@@ -11,12 +11,13 @@ export default function useControlled({ controlled, default: defaultProp, name }
       if (isControlled !== (controlled !== undefined)) {
         console.error(
           [
-            `Material-UI: A component is changing ${
-              isControlled ? 'a ' : 'an un'
-            }controlled ${name} to be ${isControlled ? 'un' : ''}controlled.`,
+            `Material-UI: a component is changing the ${
+              isControlled ? '' : 'un'
+            }controlled ${state} state of ${name} to be ${isControlled ? 'un' : ''}controlled.`,
             'Elements should not switch from uncontrolled to controlled (or vice versa).',
             `Decide between using a controlled or uncontrolled ${name} ` +
               'element for the lifetime of the component.',
+            "The nature of the state is determined during the first render, it's considered controlled if the value is not `undefined`.",
             'More info: https://fb.me/react-controlled-components',
           ].join('\n'),
         );
@@ -29,7 +30,7 @@ export default function useControlled({ controlled, default: defaultProp, name }
       if (defaultValue !== defaultProp) {
         console.error(
           [
-            `Material-UI: A component is changing the default value of an uncontrolled ${name} after being initialized. ` +
+            `Material-UI: a component is changing the default ${state} state of an uncontrolled ${name} after being initialized. ` +
               `To suppress this warning opt to use a controlled ${name}.`,
           ].join('\n'),
         );

diff --git a/packages/material-ui/src/utils/useControlled.test.js b/packages/material-ui/src/utils/useControlled.test.js
@@ -59,8 +59,8 @@ describe('useControlled', () => {
     expect(consoleErrorMock.callCount()).to.equal(0);
     setProps({ value: 'foobar' });
     expect(consoleErrorMock.callCount()).to.equal(1);
-    expect(consoleErrorMock.messages()[0]).to.contains(
-      'A component is changing an uncontrolled TestComponent to be controlled.',
+    expect(consoleErrorMock.messages()[0]).to.include(
+      'Material-UI: a component is changing the uncontrolled value state of TestComponent to be controlled.',
     );
   });
 
@@ -69,8 +69,8 @@ describe('useControlled', () => {
     expect(consoleErrorMock.callCount()).to.equal(0);
     setProps({ value: undefined });
     expect(consoleErrorMock.callCount()).to.equal(1);
-    expect(consoleErrorMock.messages()[0]).to.contains(
-      'A component is changing a controlled TestComponent to be uncontrolled.',
+    expect(consoleErrorMock.messages()[0]).to.include(
+      'Material-UI: a component is changing the controlled value state of TestComponent to be uncontrolled.',
     );
   });
 
@@ -79,8 +79,8 @@ describe('useControlled', () => {
     expect(consoleErrorMock.callCount()).to.equal(0);
     setProps({ defaultValue: 1 });
     expect(consoleErrorMock.callCount()).to.equal(1);
-    expect(consoleErrorMock.messages()[0]).to.contains(
-      'A component is changing the default value of an uncontrolled TestComponent after being initialized. ',
+    expect(consoleErrorMock.messages()[0]).to.include(
+      'Material-UI: a component is changing the default value state of an uncontrolled TestComponent after being initialized.',
     );
   });
 });