Material-ui: [docs] Document ref prop types

[cmeeren]

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

In #17097 the Popover.action prop was changed from object to func | object, but no explanation exists for what the object actually is. I understand the func callback - you get an object with an updatePosition() member you can call. But what about object? I have no idea how that is used. This should be documented. :)

[cmeeren]

This is also the case for ButtonBase.action. Upon testing, I guess object indicates you can use React.useRef. Should be documented.

Tabs.action is the same, React.useRef works, but this isn't documented.

[cmeeren]

Documenting that the object is in fact a React ref can also be considered for the following props:

• ButtonBase.buttonRef
• Checkbox.inputRef
• FilledInput.inputRef
• FormControlLabel.inputRef
• Input.inputRef
• InputBase.inputRef
• OutlinedInput.inputRef
• Popper.popperRef
• Radio.inputRef
• RootRef.rootRef
• Switch.inputRef
• TextField.inputRef

[oliviertassinari]

@cmeeren I wish we could be using https://github.com/facebook/prop-types/issues/240. I wouldn't be opposed to creating a custom prop-types now, at least, to be a placeholder for the future, when this issue gets resolved. Our version could even be used by some brave people.
Oh, and we could change the displayed type in the documentation! Saying something about a ref, this would be a significant plus 💯.

I have looked at the TypeScript part of the issue, I believe we can apply the following diffs:

diff --git a/packages/material-ui-styles/src/withStyles/withStyles.d.ts b/packages/material-ui-styles/src/withStyles/withStyles.d.ts
index 4890fab8e..8f1b821fa 100644
--- a/packages/material-ui-styles/src/withStyles/withStyles.d.ts
+++ b/packages/material-ui-styles/src/withStyles/withStyles.d.ts
@@ -95,7 +95,7 @@ export type WithStyles<
   IncludeTheme extends boolean | undefined = false
 > = (IncludeTheme extends true ? { theme: ThemeOfStyles<StylesType> } : {}) & {
   classes: ClassNameMap<ClassKeyOfStyles<StylesType>>;
-  innerRef?: React.Ref<any> | React.RefObject<any>;
+  innerRef?: React.Ref<any>;
 } & PropsOfStyles<StylesType>;

 export interface StyledComponentProps<ClassKey extends string = string> {
@@ -103,7 +103,7 @@ export interface StyledComponentProps<ClassKey extends string = string> {
    * Override or extend the styles applied to the component.
    */
   classes?: Partial<ClassNameMap<ClassKey>>;
-  innerRef?: React.Ref<any> | React.RefObject<any>;
+  innerRef?: React.Ref<any>;
 }

 export default function withStyles<
diff --git a/packages/material-ui/src/InputBase/InputBase.d.ts b/packages/material-ui/src/InputBase/InputBase.d.ts
index dad03d861..48f7668f6 100644
--- a/packages/material-ui/src/InputBase/InputBase.d.ts
+++ b/packages/material-ui/src/InputBase/InputBase.d.ts
@@ -17,7 +17,7 @@ export interface InputBaseProps
   id?: string;
   inputComponent?: React.ElementType<InputBaseComponentProps>;
   inputProps?: InputBaseComponentProps;
-  inputRef?: React.Ref<any> | React.RefObject<any>;
+  inputRef?: React.Ref<any>;
   margin?: 'dense' | 'none';
   multiline?: boolean;
   name?: string;
diff --git a/packages/material-ui/src/Popover/Popover.d.ts b/packages/material-ui/src/Popover/Popover.d.ts
index 7755b8eb1..c2c0de107 100644
--- a/packages/material-ui/src/Popover/Popover.d.ts
+++ b/packages/material-ui/src/Popover/Popover.d.ts
@@ -18,7 +18,7 @@ export type PopoverReference = 'anchorEl' | 'anchorPosition' | 'none';

 export interface PopoverProps
   extends StandardProps<ModalProps & Partial<TransitionHandlerProps>, PopoverClassKey, 'children'> {
-  action?: (actions: PopoverActions) => void;
+  action?: React.Ref<PopoverActions>;
   anchorEl?: null | Element | ((element: Element) => Element);
   anchorOrigin?: PopoverOrigin;
   anchorPosition?: PopoverPosition;
diff --git a/packages/material-ui/src/Tabs/Tabs.d.ts b/packages/material-ui/src/Tabs/Tabs.d.ts
index a070dd2b6..9414b70bd 100644
--- a/packages/material-ui/src/Tabs/Tabs.d.ts
+++ b/packages/material-ui/src/Tabs/Tabs.d.ts
@@ -4,7 +4,7 @@ import { OverridableComponent, SimplifiedPropsOf } from '../OverridableComponent

 declare const Tabs: OverridableComponent<{
   props: {
-    action?: (actions: TabsActions) => void;
+    action?: React.Ref<TabsActions>;
     centered?: boolean;
     children?: React.ReactNode;
     indicatorColor?: 'secondary' | 'primary' | string;
diff --git a/packages/material-ui/src/TextField/TextField.d.ts b/packages/material-ui/src/TextField/TextField.d.ts
index 31282f774..763788166 100644
--- a/packages/material-ui/src/TextField/TextField.d.ts
+++ b/packages/material-ui/src/TextField/TextField.d.ts
@@ -21,7 +21,7 @@ export interface BaseTextFieldProps
   helperText?: React.ReactNode;
   id?: string;
   InputLabelProps?: Partial<InputLabelProps>;
-  inputRef?: React.Ref<any> | React.RefObject<any>;
+  inputRef?: React.Ref<any>;
   label?: React.ReactNode;
   margin?: PropTypes.Margin;
   multiline?: boolean;
diff --git a/packages/material-ui/src/styles/withTheme.d.ts b/packages/material-ui/src/styles/withTheme.d.ts
index 4446f21ba..d7385fd01 100644
--- a/packages/material-ui/src/styles/withTheme.d.ts
+++ b/packages/material-ui/src/styles/withTheme.d.ts
@@ -6,7 +6,7 @@ export interface WithTheme {
 }

 export interface ThemedComponentProps extends Partial<WithTheme> {
-  innerRef?: React.Ref<any> | React.RefObject<any>;
+  innerRef?: React.Ref<any>;
 }

 declare const withTheme: PropInjector<WithTheme, ThemedComponentProps>;
diff --git a/packages/material-ui/src/withWidth/withWidth.d.ts b/packages/material-ui/src/withWidth/withWidth.d.ts
index 7037f96fa..1f8e2522e 100644
--- a/packages/material-ui/src/withWidth/withWidth.d.ts
+++ b/packages/material-ui/src/withWidth/withWidth.d.ts
@@ -13,7 +13,7 @@ export interface WithWidth {
 }

 export interface WithWidthProps extends Partial<WithWidth> {
-  innerRef?: React.Ref<any> | React.RefObject<any>;
+  innerRef?: React.Ref<any>;
 }

 export function isWidthDown(

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/396247807acac60d0a9c1ac12d55a51af4d64de7/types/react/index.d.ts#L83

[oliviertassinari]

@cmeeren Your F# fable effort seems to be very efficient at spotting holes in our components 😆. I love that!

Are you in for a new pull request?

[cmeeren]

@cmeeren Your F# fable effort seems to be very efficient at spotting holes in our components 😆. I love that!

Yeah, since I'm just generating F# bindings, the docs are everything. (I'm even auto-generating the bindings based on the final HTML docs.) So I have had to carefully look at every API component and property. :)

Are you in for a new pull request?

I have no problem creating a PR with the diffs you mentioned, but since you already have the diffs, perhaps it's quicker for you to just commit them and create a PR? I have no personal need to "own" this change, and most of my PRs have ended up with serious maintainer modifications anyway 😆 (not saying that's a bad thing, of course)

But if you'd rather I do it, I can.

[oliviertassinari]

@cmeeren Fair enough. I'm looking at it.

[cmeeren]

I notice that in the updated docs, Popper.popperRef and RootRef.rootRef are all still func | object, and Tabs.action is func. I suppose they should all be ref?

[oliviertassinari]

@cmeeren Yes 👍

[cmeeren]

Should this issue be re-opened? Should I post a new issue? Or do you have this under control?

[oliviertassinari]

@cmeeren I have personally created a reminder for it. If you want to create a new pull request with the fix, that would be great, otherwise, no I don't think that we need to create more noise by reopening. Hopefully, It will be handled, at some point.

[oliviertassinari]

I'm working on it.

[cmeeren]

Shouldn't the rootRef type be ref, not refType.isRequired?

[oliviertassinari]

Yes, this look like an edge case we can ignore.

[Izhaki]

I just wonder why this:

const refType = PropTypes.oneOfType([PropTypes.func, PropTypes.PropTypes.object]);

And not this:

const refType = (currentPropType) => PropTypes.oneOfType([
  PropTypes.func, 
  PropTypes.shape({ current: currentPropType })
]);

And then in the caller:

  inputRef: ref(PropTypes.instanceOf(Element))

For all I can tell, mui doesn't need the proposed version, but it is more 'generic'.

[oliviertassinari]

Element doesn't work server side nor cross windows.