[utils] Allow passing `NaN` as `defaultValue` to `useControlled`
- [x] I have followed (at least) the PR section of the contributing guide.
This fixes an issue with a warning getting logged when passing NaN as defaultValue to useControlled due to using === instead of Object.is. However, Object.is is not supported in IE11 so I made the change in a backwards-compatible way.
Netlify deploy preview
https://deploy-preview-41559--material-ui.netlify.app/
Bundle size report
No bundle size changes (Toolpad) No bundle size changes
Generated by :no_entry_sign: dangerJS against e364f146d068d0a4d4c60715ec62c12474eeacc4
Just curious - are all changes supposed to go into next branch starting from today, and not master anymore? If next, then I guess IE support could be dropped already.. And if not, then should this PR target master?
Could you provide a reproduction? This StackBlitz sandbox template may be a good starting point.
Simple repro with the Autocomplete component, but essentially anything that uses the useControlled hook suffers from the same issue. https://stackblitz.com/edit/stackblitz-starters-unpbpo?file=src%2FApp.tsx
Doesn't mean I think it's a good idea to use
NaNas a value to represent state, but it should probably be left to the consumer of this hook to decide how to handle it.
I agree 👍 generally we are moving towards using null for "no value" (e.g. Material UI's Select vs Base UI's Select) but it's totally the user's decision ~
(I think I've seen RA use NaN internally for "no value" as well)
Completely contrived example but you could imagine an educational calculator component that lets you pick from values like NaN, -10, 10 to see the impact of multiplying it by another hard-coded number. Sure, you could write code to treat null as NaN, but seems a bit annoying 😛
@mj12albert Since you approved it, could you merge it? Let's not leave it hanging, unless you're waiting for something specific. Should we also consider cherry-picking this to the master branch?
One example is if you have a
Selectcomponent where one of the options has value ofNaNand you set thedefaultValuetoNaN
Just occurred to me to quickly test this out:
<Select defaultValue={NaN}>
<MenuItem value={NaN}>NaN</MenuItem>
</Select>
useControlled doesn't warn, but the component logged 2 other warnings:
Warning: Received NaN for the `value` attribute. If this is expected, cast the value to a string.
at input
and
Warning: Received NaN for the `data-value` attribute. If this is expected, cast the value to a string.
at li
Though this doesn't necessarily block the PR, just wanted to ask @iammminzzy if you tried these changes with any (other) Material UI component successfully!
@iammminzzy Any updates on https://github.com/mui/material-ui/pull/41559#issuecomment-2111604547?
useControlleddoesn't warn, but the component logged 2 other warningsThough this doesn't necessarily block the PR, just wanted to ask @iammminzzy if you tried these changes with any (other) Material UI component successfully!
The stackblitz link in this comment uses an AutoComplete component which doesn't throw these warnings with NaN value https://github.com/mui/material-ui/pull/41559#issuecomment-2081473362
Personally I think we should "fix" the other two warnings as well but that's unrelated to this PR.
It looks good 👍
On making this work:
<Select defaultValue={NaN}>
<MenuItem value={NaN}>NaN</MenuItem>
</Select>
I think it's important to understand what NaN is about, from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NaN, NaN in JavaScript behaves like in IEEE-754.
So from, there, we can check why the IEEE-754 committee made it so that NaN !== NaN?
Some lights on this: https://stackoverflow.com/questions/1565164/what-is-the-rationale-for-all-comparisons-returning-false-for-ieee754-nan-values. As I read this, it's effectively:
- Before, the
isNaN(x)function didn't exist, developers would test this with x === x. This is no longer a problem. -
NaN - NaN = NaN, and notNaN - NaN = 0, soNaN != NaN. Kind of the same idea as https://stackoverflow.com/a/1565254/2801714
If I hand you two boxes, and tell you that neither of them contains an apple, would you tell me that the boxes contain the same thing? NaN contains no information about what something is, just what it isn't. Therefore these elements can never definitely be said to be equal.
So I think that NaN shouldn't be used for anything else than arithmetic errors, and definitely not for a component value. I think we could still support it but on the only condition that it doesn't harm the majority of the users, e.g. no real bundle size increase / not code complexity increase, it wouldn't be worth it if it did.