[utils] Allow passing NaN as defaultValue to useControlled
#41559
Conversation
Netlify deploy previewhttps://deploy-preview-41559--material-ui.netlify.app/ Bundle size report |
iammminzzy
changed the title
NaN as defaultValue to useControlledNaN as defaultValue to useControlled
|
Just curious - are all changes supposed to go into |
| it('should not raise a warning if setting NaN as the defaultValue when uncontrolled', () => { | ||
| expect(() => { | ||
| render(<TestComponent defaultValue={NaN}>{() => null}</TestComponent>); | ||
| }).not.toErrorDev(); | ||
| }); |
There was a problem hiding this comment.
Why should it support this? It looks like more bundle size and complexity for no clear value in exchange.
Object.is is not supported in IE11
We don't support IE 11 anymore, so maybe Object.is would make it, a light DX improvement for no real UX cost.
There was a problem hiding this comment.
One example is if you have a Select component where one of the options has value of NaN and you set the defaultValue to NaN you would incorrectly get the "component is changing the default value state" warning.
There was a problem hiding this comment.
Bundle size shouldn’t be impacted at all because the whole effect is wrapped in a process.env.NODE_ENV check, and nowhere else in the codebase uses Object.is and there are IE11 comments, so we figured it’s better to keep the partial IE11 support but more than happy to use Object.is (that’s what React uses for dependency arrays, for example)
There was a problem hiding this comment.
One example is if you have a Select component where one of the options has value of
NaN
Isn't it wrong that an option has a value of NaN?
There was a problem hiding this comment.
One example is if you have a Select component where one of the options has value of
NaNIsn't it wrong that an option has a value of
NaN?
Respectfully, I don't think there's anything wrong with using NaN as a value. For example I could use it to represent a "None" value, or maybe the number value comes from user input and NaN represents an invalid input.
oliviertassinari
added
package: base-ui
iammminzzy
force-pushed
the
use-controlled
branch
from
94adbb8
to
a330197
Compare
There was a problem hiding this comment.
Could you provide a reproduction? This StackBlitz sandbox template may be a good starting point.
Simple repro with the |
ZeeshanTamboli
added
package: utils (private)
There was a problem hiding this comment.
This seems fine to me, but since it's also used in MUI X, it should be reviewed by others too. cc @flaviendelangle @Janpot (Tagging Jan because it might be used in Toolpad too).
There was a problem hiding this comment.
I see no problems with this, the warning is about comparing values of props between renders and React also use Object.is for this purpose. It doesn't affect production bundle sizes.
Doesn't mean I think it's a good idea to use NaN as 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 |
|
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 |
This fixes an issue with a warning getting logged when passing
NaNasdefaultValuetouseControlleddue to using===instead ofObject.is. However,Object.isis not supported in IE11 so I made the change in a backwards-compatible way.