material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[utils] Allow passing `NaN` as `defaultValue` to `useControlled`

Open iammminzzy opened this issue 1 year ago • 8 comments

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.

iammminzzy avatar Mar 19 '24 12:03 iammminzzy

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

mui-bot avatar Mar 19 '24 12:03 mui-bot

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?

NMinhNguyen avatar Mar 19 '24 13:03 NMinhNguyen

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

QingqiShi avatar Apr 28 '24 12:04 QingqiShi

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 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)

mj12albert avatar May 13 '24 06:05 mj12albert

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 😛

NMinhNguyen avatar May 13 '24 15:05 NMinhNguyen

@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?

ZeeshanTamboli avatar May 14 '24 05:05 ZeeshanTamboli

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

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!

mj12albert avatar May 15 '24 05:05 mj12albert

@iammminzzy Any updates on https://github.com/mui/material-ui/pull/41559#issuecomment-2111604547?

ZeeshanTamboli avatar May 20 '24 15:05 ZeeshanTamboli

useControlled doesn't warn, but the component logged 2 other warnings

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!

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.

QingqiShi avatar Jun 04 '24 11:06 QingqiShi

@iammminzzy Any updates on #41559 (comment)?

It's the same repro as @QingqiShi shared above.

iammminzzy avatar Jun 07 '24 14:06 iammminzzy

It looks good 👍

oliviertassinari avatar Jun 08 '24 11:06 oliviertassinari

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 not NaN - NaN = 0, so NaN != 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.

oliviertassinari avatar Jun 08 '24 11:06 oliviertassinari