[core] Refactor system theme props #43120
Conversation
There was a problem hiding this comment.
As I was reading through the implementation, I noticed this function that is called in all our components via useThemeProps or other helpers had a few issues, I did a drive-by refactor here. As this is called in (I think) every render of every component we have when there are default props, I prioritized a leaner and more efficient implementation.
Related PR, for context: #35477
| const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T]; | ||
| const slotProps = props[propName] as {} as T[keyof T]; | ||
| output[propName] = {} as T[keyof T]; | ||
| for (const key in defaultProps) { |
There was a problem hiding this comment.
Switched the Object.keys for a for ... in loop, which is more efficient.
Netlify deploy previewhttps://deploy-preview-43120--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
| if (Object.prototype.hasOwnProperty.call(defaultProps, key)) { | ||
| const propName = key as keyof T; | ||
|
|
||
| if (propName === 'components' || propName === 'slots') { |
There was a problem hiding this comment.
Same for the regex name check vs string comparison. Regexes are quite efficient on modern engines but there is still some overhead: https://jsben.ch/iNyQn
I also find this a bit more readable.
| if (!slotProps || !Object.keys(slotProps)) { | ||
| // Reduce the iteration if the slot props is empty | ||
| output[propName] = defaultSlotProps; |
There was a problem hiding this comment.
!Object.keys(anything) is always going to be false, because it's equivalent to ![]. I'm guessing this was meant to be !Object.key(x).length. Anyway, I've replaced with a simpler version.
There was a problem hiding this comment.
Did one last change, removed the empty object check optimization. Avoiding iterating an empty object isn't an interesting optimization, and it's an unlikely case, and less code is always a win.
| ...(output[propName] as any), | ||
| }; | ||
| } else if (propName.toString().match(/^(componentsProps|slotProps)$/)) { | ||
| const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T]; |
There was a problem hiding this comment.
Setting defaultSlotProps to an empty object by default (the || {} part) makes the check that follows on line 31 (else if (!defaultSlotProps || !Object.keys(defaultSlotProps))) useless. In addition to the !Object.keys(x) pattern that is also present on line 28.
| } else if (propName.toString().match(/^(componentsProps|slotProps)$/)) { | ||
| const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T]; | ||
| const slotProps = props[propName] as {} as T[keyof T]; | ||
| output[propName] = {} as T[keyof T]; |
There was a problem hiding this comment.
This property is redefined in all 3 branches below, which are exhaustive, so the empty object allocated here is never used.
romgrk
force-pushed
the
fix-system-theme-props
branch
from
cee9e21
to
f289863
Compare
|
Updated the branch. What do you mean by verifying on MUI X? I used the following test code to ensure the props are now stable: https://codesandbox.io/s/angry-cray-gwhs5t And should I ignore the argos changes? |
| @@ -7,6 +8,5 @@ export default function useThemeProps({ props, name, defaultTheme, themeId }) { | |||
| if (themeId) { | |||
| theme = theme[themeId] || theme; | |||
| } | |||
| const mergedProps = getThemeProps({ theme, name, props }); | |||
| return mergedProps; | |||
| return React.useMemo(() => getThemeProps({ theme, name, props }), [theme, name, props]); | |||
There was a problem hiding this comment.
I wonder what's the stability of props itself here ?
Have you done any benchmark for this hook ?
There was a problem hiding this comment.
In the grid props is stable because we React.memo (or more exactly fastMemo) most of our components including the root one, for non-memoized components it would be stable only if the component is re-rendering because one of its hooks changed, but it wouldn't be stable if it re-renders because its parent re-rendered (the most common case I think).
Now that I think, I guess it would be possible to add the React.useMemo outside useThemeProps (in the mui-x codebase), it would avoid the cost for the rest of the components. It does feel a bit cleaner to have a stable output, but it's more performant to add it only where needed. I could be convinced either way. What do you think?
There was a problem hiding this comment.
I'd also assume that most of the times, parent will re-render making the memo useless in this case.
Only of it was using some specific keys from props instead of the whole object.
There was a problem hiding this comment.
I've thought about it some more and I prefer to add the memoization only where it's needed, so I've removed this part. The only thing left is the refactor for resolveProps, but it's a good refactor as there were some issues with the code there, so I'll merge this PR anyway.
There was a problem hiding this comment.
I'm going to proceed with the plan above, but might need to come back. Can't wrap useThemeProps with useMemo because it is itself a hook (calling useContext), and needs to be reactively bound to the theme context change.
So useMemo(() => useThemeProps({ name, props }), [name, props])) is incorrect. I have more thinking to do because for the design-system agnostic objective for the grid I need to get rid of the dependency on useThemeProps anyway, so I'll do the thinking and merge this small refactor as-is.
Yes, looks like the changes comes from the |
Thanks. |
|
@romgrk I merged the |
…i into fix-system-theme-props
romgrk
changed the title
Closes mui/mui-x#10033
When there are theme
defaultProps, the return value ofuseThemePropsis not memoized and change on every render. The grid passes those props down to is subcomponents through context, so it re-renders everything on every render.This PR makes the return props more stable.