[core] Warn when defaultValue changes #19070
Conversation
|
@material-ui/core: parsed: +0.13% , gzip: +0.22% Details of bundle changes.Comparing: 89cbce9...19aeedd
|
|
@m4theushw Thanks for looking at the issue
|
oliviertassinari
changed the title
oliviertassinari
changed the title
Yes, I didn't know this pattern was repeated.
The other approach to solve this problem is a warning. I custom hook
Without an effect, how I'll know when the |
|
@m4theushw I had forgotten about facebook/react#12710. As you can see in https://codesandbox.io/s/sad-sun-jbt4f, people shouldn't expect to be able to update the |
oliviertassinari
changed the title
oliviertassinari
added
the
PR: needs revision
|
@oliviertassinari Yes, I'll redo my PR to introduce a custom hook with the warning. Do you think is better to make this hook specifically for |
|
@m4theushw I haven't looked closely at how we could make it work. I think that it would be great if it could handle one prop with it's controlled and uncontrolled variations as well as the warning. For instance: diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 93ab41f56..c267d818a 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -371,28 +371,10 @@ const Slider = React.forwardRef(function Slider(props, ref) {
const [active, setActive] = React.useState(-1);
const [open, setOpen] = React.useState(-1);
- const { current: isControlled } = React.useRef(valueProp != null);
- const [valueState, setValueState] = React.useState(defaultValue);
- const valueDerived = 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 Slider to be ${isControlled ? 'un' : ''}controlled.`,
- 'Elements should not switch from uncontrolled to controlled (or vice versa).',
- 'Decide between using a controlled or uncontrolled Slider ' +
- 'element for the lifetime of the component.',
- 'More info: https://fb.me/react-controlled-components',
- ].join('\n'),
- );
- }
- }, [valueProp, isControlled]);
- }
+ const [valueDerived, setValueState] = useControlled({
+ controlled: valueProp,
+ default: defaultValue,
+ name: 'Slider',
+ });
const range = Array.isArray(valueDerived);
const instanceRef = React.useRef(); |
|
@oliviertassinari Nice, I'll work on your suggestion. |
m4theushw
force-pushed
the
autocomplete-defaultValue
branch
from
7cbad29 to
270db1a
Compare
m4theushw
force-pushed
the
autocomplete-defaultValue
branch
from
9e91dcb to
dfe7f36
Compare
|
It's taking shape, can't wait to review it :) |
|
@oliviertassinari Could you take a first look? I noticed that diff --git a/packages/material-ui/src/utils/useControlled.js b/packages/material-ui/src/utils/useControlled.js
index c76fb5a5d..4c86401d6 100644
--- a/packages/material-ui/src/utils/useControlled.js
+++ b/packages/material-ui/src/utils/useControlled.js
@@ -38,7 +38,13 @@ const useControlled = ({ controlled, default: defaultProp, name }) => {
}, [JSON.stringify(defaultProp)]);
}
- return { value, setValue, isControlled };
+ const setValueIfUncontrolled = newValue => {
+ if (!isControlled) {
+ setValue(newValue);
+ }
+ };
+
+ return [value, setValueIfUncontrolled];
};
export default useControlled; |
|
@m4theushw It looks awesome. I have added a test case and move the default prop change check away from production. Let us know if it sounds still look OK! Also, I have used a function over an arrow, it's more a convention we have in a the codebase (top-level => function, nested => arrow function).
This sounds great :). Do you want to make the changes? However, be careful with hook dependencies, the reference of the callback changes. |
oliviertassinari
force-pushed
the
autocomplete-defaultValue
branch
from
b058ccb to
cd55165
Compare
oliviertassinari
force-pushed
the
autocomplete-defaultValue
branch
from
cd55165 to
2478bf0
Compare
oliviertassinari
removed
the
PR: needs revision
|
@joshwooding Thanks for having a look, I'm sure your prior effect has helped too :). I suspect we will get feedback from the community on these changes, I suspect that a non-negligeable amount of project have cases where the default prop changes. I hope developers don't provide object with circular references, which would break |
Fixes #18956
This PR adds support to update the value when the
defaultValueprop is changed after initial rendering. New default values are only propagated when the component is not dirty, to preserve the values the user may already have selected.