[Select] Out of range defaultValue causes onChange to fire on mount

[DiegoAndai]

Duplicates

• [X] I have searched the existing issues

Latest version

• [X] I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/use-select-default-value-triggers-on-change-h5mmmh?file=/Demo.js

Steps:

1. Create a Select component using the useSelect hook
2. Provide an onChange callback to useSelect
3. Provide a defaultValue to useSelect which doesn't correspond to any of the options' values

Current behavior 😯

When the component mounts, the onChange callback is fired with null values for the event and newValue parameters.

Expected behavior 🤔

• The onChange callback shouldn't fire
• There should be a warning about the value provided to useSelect being out of range

Context 🔦

Refactoring material-ui v5 Select to be based on the useSelect hook. Hopefully, we can make it so that the refactored version fails gracefully when value = "" or defaultValue = "" as that was the way to deselect all options in v5.

Your environment 🌎

npx @mui/envinfo

``` System: OS: macOS 13.4.1 Binaries: Node: 18.16.1 - ~/.nvm/versions/node/v18.16.1/bin/node Yarn: 1.22.19 - ~/.nvm/versions/node/v18.16.1/bin/yarn npm: 9.5.1 - ~/.nvm/versions/node/v18.16.1/bin/npm Browsers: Chrome: 117.0.5938.149 Edge: Not Found Safari: 16.5.1 npmPackages: @emotion/react: 11.11.1 @emotion/styled: 11.11.0 @mui/base: 5.0.0-beta.17 @mui/codemod: 5.14.11 @mui/core-downloads-tracker: 5.14.11 @mui/docs: 5.14.11 @mui/envinfo: 2.0.10 @mui/icons-material: 5.14.11 @mui/internal-waterfall: 1.0.0 @mui/joy: 5.0.0-beta.8 @mui/lab: 5.0.0-alpha.146 @mui/markdown: 5.0.0 @mui/material: 5.14.11 @mui/material-next: 6.0.0-alpha.103 @mui/private-theming: 5.14.11 @mui/styled-engine: 5.14.11 @mui/styled-engine-sc: 5.14.11 @mui/styles: 5.14.11 @mui/system: 5.14.11 @mui/types: 7.2.4 @mui/utils: 5.14.11 @mui/x-charts: 6.0.0-alpha.12 @mui/x-data-grid: 6.15.0 @mui/x-data-grid-generator: 6.15.0 @mui/x-data-grid-premium: 6.15.0 @mui/x-data-grid-pro: 6.15.0 @mui/x-date-pickers: 6.15.0 @mui/x-date-pickers-pro: 6.15.0 @mui/x-license-pro: 6.10.2 @mui/x-tree-view: 6.0.0-alpha.1 @mui/zero-jest: 0.0.1-alpha.0 @mui/zero-next-plugin: 0.0.1-alpha.0 @mui/zero-runtime: 0.0.1-alpha.0 @mui/zero-tag-processor: 0.0.1-alpha.0 @mui/zero-vite-plugin: 0.0.1-alpha.0 @types/react: ^18.2.23 => 18.2.23 react: 18.2.0 react-dom: 18.2.0 styled-components: 5.3.11 typescript: ^5.1.6 => 5.1.6 ```

[michaldudak]

A warning here is a good idea, but I did the resetting to no selected value when an invalid one is provided on purpose. Of course, whether it leads to better or worse DX depends on the use case, so I'd wait for additional feedback before changing it. For Material UI, would changing "" to null before passing to useSelect be an option?

[DiegoAndai]

For Material UI, would changing "" to null before passing to useSelect be an option?

Yeah 🙌🏼 that works.

I did the resetting to no selected value when an invalid one is provided on purpose

What confused me was that the value change that caused the onChange is "hidden" from the developer. I didn't understand why the value changed; it wasn't changing on my side, so I had to dig into the useSelect/useList code to understand why.

This unexpectedonChange firing with no event and no new value might frustrate developers who need to debug it. In my case, the onChange was bubbling up to the Select's related Input, which was throwing an error as the event came as null.

I think the root of this is that the initial state's defaultValue is not checked against the available options (here), while the selected values are (here). My expectation was for both to be checked.

[michaldudak]

This unexpectedonChange firing with no event and no new value might frustrate developers who need to debug it. In my case, the onChange was bubbling up to the Select's related Input, which was throwing an error as the event came as null.

I think the root of this is that the initial state's defaultValue is not checked against the available options (here), while the selected values are (here). My expectation was for both to be checked.

Gotcha. Do you think a warning in the console would be sufficient to help debug these issues?

I'm not strictly against not firing the onChange. I just imagine issues complaining that onChange wasn't called despite the selected value changed. I suppose there can be some confusion either way.

BTW, it may turn out we need to support nonexistent values somehow to solve mui/base-ui#37, but we haven't investigated possible implementations yet.

[DiegoAndai]

Do you think a warning in the console would be sufficient to help debug these issues?

It would be an improvement, but it would still be up to the developer to connect the "Warning nonexistent value" with the onChange firing.

I'm not strictly against not firing the onChange. I just imagine issues complaining that onChange wasn't called despite the selected value changed. I suppose there can be some confusion either way.

I agree that if the value changes, then onChange should fire.

For the "defaultValue prop is a nonexistent value" case, it makes sense to set the value to null as we're in uncontrolled mode. What is weird to me, in this case, is that useSelect initially returns value = defaultValue, then we notice there is a nonexistent value, so we filter it, and then useSelect returns value = null and onChange is called with newValue = null. If we filtered initially so value = null since the start, then onChange would have no reason to fire. Is this a limitation due to the options registering asynchronously? We wouldn't know if it's nonexistent before the options register.

For the "value prop is a nonexistent value" case, I think it's okay to fire onChange, but it could be improved by documenting that this "nonexistent value prop" event will cause onChange to fire. We could improve it more if we gave the developer a way to differentiate if the onChange fired because of a user's selection or because the value prop provided is nonexistent. So maybe adding a third parameter, reason?

I wonder how this behavior would affect the case of async loading of options: for example, a Select with virtualization, if the user selects a value and then scrolls the listbox, making the selected option unmount, would we trigger onChange with newValue = null?

[aacevski]

Hello @michaldudak, I'd like to tackle this however what do you think is the best solution?

Imo not triggering the onChange and showing a warning is fine for me - is there any additional things I need to do?

[michaldudak]

The Select will soon undergo a severe API change, which may affect its internals. Let's not spend time on fixing its bugs till then (as it may be fixed during the API change).