Material-ui: [Select] node is undefined

[ffjanhoeck]

• [ x ] This is not a v0.x issue.
• [ x ] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

The expected behavior is to get the node target of an text field with select={true}.

Current Behavior 😯

If you pass down a ref to a TextField by using inputProps, you will get the select component.
This object has a node key. But in the latest version of material-ui it is always empty.
We use it for field validations.

Steps to Reproduce 🕹

Visit this sandbox and open your console. If you change now the value of the select, you will see that the node key has a value (if you are using 3.9.3). But if you switch to 4.1.1 and repeat the task, then you see that the node key has no value.

https://codesandbox.io/s/vigorous-http-c7x5j

Big thanks for your help!

Your Environment 🌎

| Tech | Version |
|--------------|---------|
| Material-UI | v4.1.1 |
| React | 16.8.6 |
| Browser | Chrome |
| TypeScript | no |
| etc. | |

[oliviertassinari]

@ffjanhoeck Thanks for the report. This looks like a regression to me. What do you think of this fix?

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 6ea5b3959..5c8cedd24 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -27,7 +27,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     disabled,
     displayEmpty,
     IconComponent,
-    inputRef,
+    inputRef: inputRefProp,
     MenuProps = {},
     multiple,
     name,
@@ -48,12 +48,14 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
     ...other
   } = props;
   const displayRef = React.useRef(null);
+  const inputRef = React.useRef(null);
   const ignoreNextBlur = React.useRef(false);
   const { current: isOpenControlled } = React.useRef(openProp != null);
   const [menuMinWidthState, setMenuMinWidthState] = React.useState();
   const [openState, setOpenState] = React.useState(false);
   const [, forceUpdate] = React.useState(0);
-  const handleRef = useForkRef(ref, inputRef);
+  const handleInputRef = useForkRef(inputRef, inputRefProp);
+  const handleRef = useForkRef(ref, handleInputRef);

   React.useImperativeHandle(
     handleRef,
@@ -61,10 +63,10 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       focus: () => {
         displayRef.current.focus();
       },
-      node: inputRef ? inputRef.current : null,
+      node: inputRef.current,
       value,
     }),
-    [inputRef, value],
+    [value],
   );

   React.useEffect(() => {

Do you want to submit a pull request? :)

[ffjanhoeck]

@oliviertassinari Thanks for your answer! I would love it to submit a pull request!
But before I can submit my first one, I have some questions:

• Why do you manipulate the value of the ref using useImperativeHandle. Is that really necessary?
• Does it make sense in your opinion, to directly pass the ref into the input, without useForkRef and useImperativeHandle? I don't get it, why you are using this :)

Thank you and have a good day!

[oliviertassinari]

@ffjanhoeck Yes, useImperativeHandle and all the "mess" that comes with it is really necessary, there is no simplification potential I'm aware of. There is no input we can apply the ref on. We have to simulate it.

[ffjanhoeck]

@oliviertassinari What is the best way to test the change?
Do you always setup a new project to test your components or do you just write tests?

EDIT: Nevermind, I have found this: https://github.com/mui-org/material-ui/blob/master/CONTRIBUTING.md

[ffjanhoeck]

@oliviertassinari I fixed the bug, but in a other way as your example. Never mind, I cannot push my change to a new branch. I always get the error, that I don't have permissions. I did in the same way, like the documentation describes it.

Do you know what I do wrong?

[joshwooding]

@ffjanhoeck You need to fork the repo and create a pull request from your fork :)

[ffjanhoeck]

@joshwooding Thanks for your advice :) I directly cloned the repo. Let me try the way with the fork :)

[ffjanhoeck]

@oliviertassinari Damn it feels good... There is my first PR :-)

[ffjanhoeck]

@oliviertassinari you didn't liked my change? 😅😅

[oliviertassinari]

useForkRef was designed for this use case, it's consistent with the rest of the codebase this way.

[ffjanhoeck]

useForkRef was designed for this use case, it's consistent with the rest of the codebase this way.

Good to know :) I am looking forward to do more pull request and learn how everything works in material-ui. Hopefully my pull-request with your changes will be published fast as a new version, so we can fix our validations in our production software.

[oliviertassinari]

@ffjanhoeck It should be released this weekend.

[ffjanhoeck]

Hey @oliviertassinari , This solution does not work quite well :/
Take a look at this sandbox: https://codesandbox.io/s/angry-leaf-njnfn

Open your console and change the value of the select multiple times. As you can see, the node reference gets nullified. Do you have an idea why this happen?

[ffjanhoeck]

I am not able to reopen this issue

[oliviertassinari]

Oh yes, correct, my proposed fix was plain wrong, this works better:

diff --git a/packages/material-ui/src/Select/SelectInput.js b/packages/material-ui/src/Select/SelectInput.js
index 2811fb164..9abcb8565 100644
--- a/packages/material-ui/src/Select/SelectInput.js
+++ b/packages/material-ui/src/Select/SelectInput.js
@@ -55,8 +55,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
   const [menuMinWidthState, setMenuMinWidthState] = React.useState();
   const [openState, setOpenState] = React.useState(false);
   const [, forceUpdate] = React.useState(0);
-  const handleInputRef = useForkRef(inputRef, inputRefProp);
-  const handleRef = useForkRef(ref, handleInputRef);
+  const handleRef = useForkRef(ref, inputRefProp);

   React.useImperativeHandle(
     handleRef,
@@ -285,7 +284,7 @@ const SelectInput = React.forwardRef(function SelectInput(props, ref) {
       <input
         value={Array.isArray(value) ? value.join(',') : value}
         name={name}
-        ref={handleRef}
+        ref={inputRef}
         type={type}
         autoFocus={autoFocus}
         {...other}

What do you think?

[ffjanhoeck]

@oliviertassinari Looks good! Do you have time to make this change? Because currently I am at work and I don't have that much time this week :/ Otherwise if you don't have time too, I will try to get some time :D

[ffjanhoeck]

@oliviertassinari https://github.com/mui-org/material-ui/pull/16401
Here we go