Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[material-ui] Element ref access React 19 compatibility #43132

Merged
merged 5 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions packages/mui-base/src/ClickAwayListener/ClickAwayListener.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
unstable_ownerDocument as ownerDocument,
unstable_useForkRef as useForkRef,
unstable_useEventCallback as useEventCallback,
unstable_getElementRef as getElementRef,
} from '@mui/utils';

// TODO: return `EventHandlerName extends `on${infer EventName}` ? Lowercase<EventName> : never` once generatePropTypes runs with TS 4.1
Expand Down Expand Up @@ -94,11 +95,7 @@ function ClickAwayListener(props: ClickAwayListenerProps): React.JSX.Element {
};
}, []);

const handleRef = useForkRef(
// @ts-expect-error TODO upstream fix
children.ref,
nodeRef,
);
const handleRef = useForkRef(getElementRef(children), nodeRef);

// The handler doesn't take event.defaultPrevented into account:
//
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-base/src/FocusTrap/FocusTrap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
elementAcceptingRef,
unstable_useForkRef as useForkRef,
unstable_ownerDocument as ownerDocument,
unstable_getElementRef as getElementRef,
} from '@mui/utils';
import { FocusTrapProps } from './FocusTrap.types';

Expand Down Expand Up @@ -152,8 +153,7 @@ function FocusTrap(props: FocusTrapProps): React.JSX.Element {
const activated = React.useRef(false);

const rootRef = React.useRef<HTMLElement>(null);
// @ts-expect-error TODO upstream fix
const handleRef = useForkRef(children.ref, rootRef);
const handleRef = useForkRef(getElementRef(children), rootRef);
const lastKeydown = React.useRef<KeyboardEvent | null>(null);

React.useEffect(() => {
Expand Down
7 changes: 5 additions & 2 deletions packages/mui-base/src/Portal/Portal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import * as React from 'react';
import * as ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import getElementRef from '@mui/utils/getElementRef';
import {
exactProp,
HTMLElementType,
Expand Down Expand Up @@ -33,8 +34,10 @@ const Portal = React.forwardRef(function Portal(
) {
const { children, container, disablePortal = false } = props;
const [mountNode, setMountNode] = React.useState<ReturnType<typeof getContainer>>(null);
// @ts-expect-error TODO upstream fix
const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, forwardedRef);
const handleRef = useForkRef(
React.isValidElement(children) ? getElementRef(children) : null,
forwardedRef,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const handleRef = useForkRef(
React.isValidElement(children) ? getElementRef(children) : null,
forwardedRef,
);
const handleRef = useForkRef(getElementRef(children), forwardedRef);

We can likely simplify, as the getElementRef handles this already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored to handle this case and renamed the util accordingly 😊


useEnhancedEffect(() => {
if (!disablePortal) {
Expand Down
6 changes: 2 additions & 4 deletions packages/mui-joy/src/Tooltip/Tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
unstable_useId as useId,
unstable_useTimeout as useTimeout,
unstable_Timeout as Timeout,
unstable_getElementRef as getElementRef,
} from '@mui/utils';
import { Popper, unstable_composeClasses as composeClasses } from '@mui/base';
import { OverridableComponent } from '@mui/types';
Expand Down Expand Up @@ -415,10 +416,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) {
}, [handleClose, open]);

const handleUseRef = useForkRef(setChildNode, ref);
const handleRef = useForkRef(
(children as unknown as { ref: React.Ref<HTMLElement> }).ref,
handleUseRef,
);
const handleRef = useForkRef(getElementRef(children), handleUseRef);

// There is no point in displaying an empty tooltip.
if (typeof title !== 'number' && !title) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
unstable_useForkRef as useForkRef,
unstable_useEventCallback as useEventCallback,
} from '@mui/utils';
import getElementRef from '@mui/utils/getElementRef';

// TODO: return `EventHandlerName extends `on${infer EventName}` ? Lowercase<EventName> : never` once generatePropTypes runs with TS 4.1
function mapEventPropToEvent(
Expand Down Expand Up @@ -95,11 +96,7 @@ function ClickAwayListener(props: ClickAwayListenerProps): React.JSX.Element {
};
}, []);

const handleRef = useForkRef(
// @ts-expect-error TODO upstream fix
children.ref,
nodeRef,
);
const handleRef = useForkRef(getElementRef(children), nodeRef);

// The handler doesn't take event.defaultPrevented into account:
//
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Fade/Fade.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import { Transition } from 'react-transition-group';
import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import getElementRef from '@mui/utils/getElementRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

const styles = {
entering: {
Expand Down Expand Up @@ -49,7 +49,7 @@ const Fade = React.forwardRef(function Fade(props, ref) {

const enableStrictModeCompat = true;
const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);
const handleRef = useForkRef(nodeRef, getElementRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Grow/Grow.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import useTimeout from '@mui/utils/useTimeout';
import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import getElementRef from '@mui/utils/getElementRef';
import { Transition } from 'react-transition-group';
import { useTheme } from '../zero-styled';
import { getTransitionProps, reflow } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

function getScale(value) {
return `scale(${value}, ${value ** 2})`;
Expand Down Expand Up @@ -62,7 +62,7 @@ const Grow = React.forwardRef(function Grow(props, ref) {
const theme = useTheme();

const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);
const handleRef = useForkRef(nodeRef, getElementRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
7 changes: 5 additions & 2 deletions packages/mui-material/src/Portal/Portal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
unstable_useEnhancedEffect as useEnhancedEffect,
unstable_useForkRef as useForkRef,
unstable_setRef as setRef,
unstable_getElementRef as getElementRef,
} from '@mui/utils';
import { PortalProps } from './Portal.types';

Expand All @@ -33,8 +34,10 @@ const Portal = React.forwardRef(function Portal(
) {
const { children, container, disablePortal = false } = props;
const [mountNode, setMountNode] = React.useState<ReturnType<typeof getContainer>>(null);
// @ts-expect-error TODO upstream fix
const handleRef = useForkRef(React.isValidElement(children) ? children.ref : null, forwardedRef);
const handleRef = useForkRef(
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
React.isValidElement(children) ? getElementRef(children) : null,
forwardedRef,
);

useEnhancedEffect(() => {
if (!disablePortal) {
Expand Down
3 changes: 2 additions & 1 deletion packages/mui-material/src/Select/Select.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import deepmerge from '@mui/utils/deepmerge';
import getElementRef from '@mui/utils/getElementRef';
import SelectInput from './SelectInput';
import formControlState from '../FormControl/formControlState';
import useFormControl from '../FormControl/useFormControl';
Expand Down Expand Up @@ -85,7 +86,7 @@ const Select = React.forwardRef(function Select(inProps, ref) {
filled: <StyledFilledInput ownerState={ownerState} />,
}[variant];

const inputComponentRef = useForkRef(ref, InputComponent.ref);
const inputComponentRef = useForkRef(ref, getElementRef(InputComponent));

return (
<React.Fragment>
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Slide/Slide.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import { Transition } from 'react-transition-group';
import chainPropTypes from '@mui/utils/chainPropTypes';
import HTMLElementType from '@mui/utils/HTMLElementType';
import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import getElementRef from '@mui/utils/getElementRef';
import debounce from '../utils/debounce';
import useForkRef from '../utils/useForkRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import { ownerWindow } from '../utils';
import getChildRef from '../utils/getChildRef';

// Translate the node so it can't be seen on the screen.
// Later, we're going to translate the node back to its original location with `none`.
Expand Down Expand Up @@ -120,7 +120,7 @@ const Slide = React.forwardRef(function Slide(props, ref) {
} = props;

const childrenRef = React.useRef(null);
const handleRef = useForkRef(getChildRef(children), childrenRef, ref);
const handleRef = useForkRef(getElementRef(children), childrenRef, ref);

const normalizedTransitionCallback = (callback) => (isAppearing) => {
if (callback) {
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Tooltip/Tooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { alpha } from '@mui/system/colorManipulator';
import { useRtl } from '@mui/system/RtlProvider';
import isFocusVisible from '@mui/utils/isFocusVisible';
import appendOwnerState from '@mui/utils/appendOwnerState';
import getElementRef from '@mui/utils/getElementRef';
import { styled, useTheme } from '../zero-styled';
import { useDefaultProps } from '../DefaultPropsProvider';
import capitalize from '../utils/capitalize';
Expand All @@ -19,7 +20,6 @@ import useForkRef from '../utils/useForkRef';
import useId from '../utils/useId';
import useControlled from '../utils/useControlled';
import tooltipClasses, { getTooltipUtilityClass } from './tooltipClasses';
import getChildRef from '../utils/getChildRef';

function round(value) {
return Math.round(value * 1e5) / 1e5;
Expand Down Expand Up @@ -546,7 +546,7 @@ const Tooltip = React.forwardRef(function Tooltip(inProps, ref) {
};
}, [handleClose, open]);

const handleRef = useForkRef(getChildRef(children), setChildNode, ref);
const handleRef = useForkRef(getElementRef(children), setChildNode, ref);

// There is no point in displaying an empty tooltip.
// So we exclude all falsy values, except 0, which is valid.
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Unstable_TrapFocus/FocusTrap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
elementAcceptingRef,
unstable_useForkRef as useForkRef,
unstable_ownerDocument as ownerDocument,
unstable_getElementRef as getElementRef,
} from '@mui/utils';
import { FocusTrapProps } from './FocusTrap.types';

Expand Down Expand Up @@ -144,8 +145,7 @@ function FocusTrap(props: FocusTrapProps): React.JSX.Element {
const activated = React.useRef(false);

const rootRef = React.useRef<HTMLElement>(null);
// @ts-expect-error TODO upstream fix
const handleRef = useForkRef(children.ref, rootRef);
const handleRef = useForkRef(getElementRef(children), rootRef);
const lastKeydown = React.useRef<KeyboardEvent | null>(null);

React.useEffect(() => {
Expand Down
4 changes: 2 additions & 2 deletions packages/mui-material/src/Zoom/Zoom.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import * as React from 'react';
import PropTypes from 'prop-types';
import { Transition } from 'react-transition-group';
import elementAcceptingRef from '@mui/utils/elementAcceptingRef';
import getElementRef from '@mui/utils/getElementRef';
import { useTheme } from '../zero-styled';
import { reflow, getTransitionProps } from '../transitions/utils';
import useForkRef from '../utils/useForkRef';
import getChildRef from '../utils/getChildRef';

const styles = {
entering: {
Expand Down Expand Up @@ -49,7 +49,7 @@ const Zoom = React.forwardRef(function Zoom(props, ref) {
} = props;

const nodeRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, getChildRef(children), ref);
const handleRef = useForkRef(nodeRef, getElementRef(children), ref);

const normalizedTransitionCallback = (callback) => (maybeIsAppearing) => {
if (callback) {
Expand Down
1 change: 0 additions & 1 deletion packages/mui-material/src/utils/getChildRef.d.ts

This file was deleted.

10 changes: 0 additions & 10 deletions packages/mui-material/src/utils/getChildRef.js

This file was deleted.

20 changes: 20 additions & 0 deletions packages/mui-utils/src/getElementRef/getElementRef.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import * as React from 'react';

/**
* Returns the ref of an element handling differences between React 19 and older versions
*
* @param element React.ReactElement<any>
* @returns React.Ref<any> | null
*/
export default function getElementRef(element: React.ReactElement<any>): React.Ref<any> | null {
if (!element || !React.isValidElement(element)) {
return null;
}

// 'ref' is passed as prop in React 19, whereas 'ref' is directly attached to children in older versions
return (element.props as any).propertyIsEnumerable('ref')
? (element.props as any).ref
: // @ts-expect-error element.ref is not included in the ReactElement type
// We cannot check for it, but isValidElement is true at this point
Copy link
Member Author

@DiegoAndai DiegoAndai Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ref property does not exist in ReactElement. We've silenced this error in our codebase since: https://github.com/mui/material-ui/pull/24565/files#diff-dcdcbff6b975b552227833f729bf59d31e97e4876a9d6ce982a07c9e1436d9bfR90

Here are other occurrences, probably copy-pasted: https://github.com/search?q=repo%3Amui%2Fmaterial-ui%20%22TODO%20upstream%20fix%22&type=code

I don't think they'll fix it now that they moved the ref to the props.

I don't know if there's a better way to handle this, I couldn't find one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just recommend if you find an open issue about this, to link in the comment above so we know once it's merged that we can remove the @ts-ignore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any issue in the original PR, DefinitelyTyped issues, or DefinitelyTyped discussions. I don't know if they have discussed it previously. I don't think it will get much traction as it's probably been like this forever and it's now removed in React 19.

I created a discussion asking for this and linked it.

element.ref;
}
1 change: 1 addition & 0 deletions packages/mui-utils/src/getElementRef/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './getElementRef';
1 change: 1 addition & 0 deletions packages/mui-utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,5 @@ export { default as unstable_useSlotProps } from './useSlotProps';
export type { UseSlotPropsParameters, UseSlotPropsResult } from './useSlotProps';
export { default as unstable_resolveComponentProps } from './resolveComponentProps';
export { default as unstable_extractEventHandlers } from './extractEventHandlers';
export { default as unstable_getElementRef } from './getElementRef';
export * from './types';
3 changes: 2 additions & 1 deletion packages/mui-utils/src/useForkRef/useForkRef.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';
import { expect } from 'chai';
import { createRenderer, screen } from '@mui/internal-test-utils';
import useForkRef from './useForkRef';
import getElementRef from '../getElementRef';

describe('useForkRef', () => {
const { render } = createRenderer();
Expand Down Expand Up @@ -47,7 +48,7 @@ describe('useForkRef', () => {
it('does nothing if none of the forked branches requires a ref', () => {
const Outer = React.forwardRef(function Outer(props, ref) {
const { children } = props;
const handleRef = useForkRef(children.ref, ref);
const handleRef = useForkRef(getElementRef(children), ref);

return React.cloneElement(children, { ref: handleRef });
});
Expand Down