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

iammminzzy · 2024-03-19T12:54:09Z

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.

mui-bot · 2024-03-19T12:56:53Z

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 🚫 dangerJS against e364f14

NMinhNguyen · 2024-03-19T13:45:09Z

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?

oliviertassinari · 2024-03-19T14:56:31Z

packages/mui-utils/src/useControlled/useControlled.test.js

+  it('should not raise a warning if setting NaN as the defaultValue when uncontrolled', () => {
+    expect(() => {
+      render(<TestComponent defaultValue={NaN}>{() => null}</TestComponent>);
+    }).not.toErrorDev();
+  });


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.

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.

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)

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?

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?

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.

ZeeshanTamboli

Could you provide a reproduction? This StackBlitz sandbox template may be a good starting point.

QingqiShi · 2024-04-28T12:44:56Z

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

ZeeshanTamboli

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

Janpot

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.

mj12albert · 2024-05-13T06:55:24Z

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)

NMinhNguyen · 2024-05-13T15:45:32Z

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 😛

ZeeshanTamboli · 2024-05-14T05:10:26Z

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

mj12albert · 2024-05-15T05:17:29Z

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!

ZeeshanTamboli · 2024-05-20T15:05:47Z

@iammminzzy Any updates on #41559 (comment)?

QingqiShi · 2024-06-04T11:01:48Z

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 #41559 (comment)

Personally I think we should "fix" the other two warnings as well but that's unrelated to this PR.

iammminzzy · 2024-06-07T14:02:22Z

@iammminzzy Any updates on #41559 (comment)?

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

) Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>

oliviertassinari · 2024-06-08T11:31:06Z

It looks good 👍

oliviertassinari · 2024-06-08T11:49:06Z

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.

…#41559) Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>

iammminzzy changed the title ~~[utils] Allow passing NaN as defaultValue to useControlled~~ [utils] Allow passing NaN as defaultValue to useControlled Mar 19, 2024

zannager added the package: material-ui Specific to @mui/material label Mar 19, 2024

zannager requested a review from mj12albert March 19, 2024 13:15

oliviertassinari reviewed Mar 19, 2024

View reviewed changes

oliviertassinari added package: base-ui Specific to @mui/base and removed package: material-ui Specific to @mui/material labels Mar 19, 2024

[utils] Allow passing NaN as defaultValue to useControlled

a330197

iammminzzy force-pushed the use-controlled branch from 94adbb8 to a330197 Compare March 20, 2024 00:34

ZeeshanTamboli reviewed Apr 27, 2024

View reviewed changes

Merge branch 'next' into use-controlled

ea98fb5

ZeeshanTamboli added package: utils Specific to the @mui/utils package enhancement This is not a bug, nor a new feature and removed package: base-ui Specific to @mui/base labels Apr 30, 2024

ZeeshanTamboli reviewed Apr 30, 2024

View reviewed changes

ZeeshanTamboli requested review from Janpot and flaviendelangle April 30, 2024 08:54

Janpot reviewed Apr 30, 2024

View reviewed changes

mj12albert approved these changes May 13, 2024

View reviewed changes

ZeeshanTamboli requested a review from mj12albert June 5, 2024 13:34

Merge branch 'next' into use-controlled

e364f14

ZeeshanTamboli added the needs cherry-pick The PR should be cherry-picked to master after merge label Jun 8, 2024

ZeeshanTamboli approved these changes Jun 8, 2024

View reviewed changes

ZeeshanTamboli merged commit c66a51d into mui:next Jun 8, 2024
20 checks passed

github-actions bot pushed a commit that referenced this pull request Jun 8, 2024

[utils] Allow passing NaN as defaultValue to useControlled (#41559

f89d5eb

) Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>

github-actions bot mentioned this pull request Jun 8, 2024

[utils] Allow passing NaN as defaultValue to useControlled (@iammminzzy) #42571

Merged

joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024

[utils] Allow passing NaN as defaultValue to useControlled (mui…

02dd848

…#41559) Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iammminzzy commented Mar 19, 2024 •

edited

Loading

mui-bot commented Mar 19, 2024 •

edited

Loading

NMinhNguyen commented Mar 19, 2024 •

edited

Loading

oliviertassinari Mar 19, 2024 •

edited

Loading

QingqiShi Mar 19, 2024

NMinhNguyen Mar 19, 2024 •

edited

Loading

ZeeshanTamboli Mar 20, 2024

QingqiShi Mar 20, 2024

ZeeshanTamboli left a comment

QingqiShi commented Apr 28, 2024 •

edited

Loading

ZeeshanTamboli left a comment

Janpot left a comment

mj12albert commented May 13, 2024

NMinhNguyen commented May 13, 2024

ZeeshanTamboli commented May 14, 2024

mj12albert commented May 15, 2024 •

edited

Loading

ZeeshanTamboli commented May 20, 2024

QingqiShi commented Jun 4, 2024

iammminzzy commented Jun 7, 2024 •

edited

Loading

oliviertassinari commented Jun 8, 2024

oliviertassinari commented Jun 8, 2024 •

edited

Loading

[utils] Allow passing NaN as defaultValue to useControlled #41559

[utils] Allow passing NaN as defaultValue to useControlled #41559

Conversation

iammminzzy commented Mar 19, 2024 • edited Loading

mui-bot commented Mar 19, 2024 • edited Loading

Netlify deploy preview

Bundle size report

NMinhNguyen commented Mar 19, 2024 • edited Loading

oliviertassinari Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

QingqiShi Mar 19, 2024

Choose a reason for hiding this comment

NMinhNguyen Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Mar 20, 2024

Choose a reason for hiding this comment

QingqiShi Mar 20, 2024

Choose a reason for hiding this comment

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

QingqiShi commented Apr 28, 2024 • edited Loading

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

Janpot left a comment

Choose a reason for hiding this comment

mj12albert commented May 13, 2024

NMinhNguyen commented May 13, 2024

ZeeshanTamboli commented May 14, 2024

mj12albert commented May 15, 2024 • edited Loading

ZeeshanTamboli commented May 20, 2024

QingqiShi commented Jun 4, 2024

iammminzzy commented Jun 7, 2024 • edited Loading

oliviertassinari commented Jun 8, 2024

oliviertassinari commented Jun 8, 2024 • edited Loading

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

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

iammminzzy commented Mar 19, 2024 •

edited

Loading

mui-bot commented Mar 19, 2024 •

edited

Loading

NMinhNguyen commented Mar 19, 2024 •

edited

Loading

oliviertassinari Mar 19, 2024 •

edited

Loading

NMinhNguyen Mar 19, 2024 •

edited

Loading

QingqiShi commented Apr 28, 2024 •

edited

Loading

mj12albert commented May 15, 2024 •

edited

Loading

iammminzzy commented Jun 7, 2024 •

edited

Loading

oliviertassinari commented Jun 8, 2024 •

edited

Loading