-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[utils] Do not deep merge React component #45058
[utils] Do not deep merge React component #45058
Conversation
Netlify deploy previewhttps://deploy-preview-45058--material-ui.netlify.app/ Grow: parsed: +2.26% , gzip: +2.16% Bundle size reportDetails of bundle changes (Toolpad) |
{ defaultProps: { component: Link } }, | ||
); | ||
|
||
expect(result.defaultProps.component).to.equal(Link); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the change in this PR, this test throws because the component is a new object. This test ensure that the Link is not iterated by deepmerge
.
@@ -21,7 +22,7 @@ export interface DeepmergeOptions { | |||
} | |||
|
|||
function deepClone<T>(source: T): T | Record<keyof any, unknown> { | |||
if (React.isValidElement(source) || !isPlainObject(source)) { | |||
if (React.isValidElement(source) || isValidElementType(source) || !isPlainObject(source)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@siriwatknp why does React.isValidElement(source)
not cover this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React.isValidElement
check React element like this <Icon />
but it does not check for React component.
createTheme({
components: {
MuiCheckbox: {
defaultProps: {
icon: <InfoSharp />, // React.isValidElement cover this
component: CustomComponent, // React.isValidElement DOES not cover this
},
},
},
});
Tested locally, it fixes the issue. |
@@ -1,4 +1,5 @@ | |||
import * as React from 'react'; | |||
import { isValidElementType } from 'react-is'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-is
is already a direct dependency of @mui/utils
.
closes #45052
Root cause
deepmerge
currently iterate a React component that's wrapped withforwardRef
ormemo
. The issue arise when using Link from nextjs (contains recursive render) becausedeepmerge
will iterate infinitely causing maximum callstack error from the usage below:It does not make sense anyway to iterate fields inside a React component, so a side benefit of this PR is the performance improvement.