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

[core] Refactor system theme props #43120

Merged
merged 7 commits into from
Aug 9, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 1 addition & 2 deletions packages/mui-system/src/useThemeProps/useThemeProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,5 @@ export default function useThemeProps({ props, name, defaultTheme, themeId }) {
if (themeId) {
theme = theme[themeId] || theme;
}
const mergedProps = getThemeProps({ theme, name, props });
return mergedProps;
return getThemeProps({ theme, name, props });
}
65 changes: 35 additions & 30 deletions packages/mui-utils/src/resolveProps/resolveProps.ts
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* Add keys, values of `defaultProps` that does not exist in `props`
* @param {object} defaultProps
* @param {object} props
* @returns {object} resolved props
* @param defaultProps
* @param props
* @returns resolved props
*/
export default function resolveProps<
T extends {
Expand All @@ -14,36 +14,41 @@ export default function resolveProps<
>(defaultProps: T, props: T) {
const output = { ...props };

(Object.keys(defaultProps) as Array<keyof T>).forEach((propName) => {
if (propName.toString().match(/^(components|slots)$/)) {
output[propName] = {
...(defaultProps[propName] as any),
...(output[propName] as any),
};
} else if (propName.toString().match(/^(componentsProps|slotProps)$/)) {
const defaultSlotProps = (defaultProps[propName] || {}) as T[keyof T];
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

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.

const slotProps = props[propName] as {} as T[keyof T];
output[propName] = {} as T[keyof T];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This property is redefined in all 3 branches below, which are exhaustive, so the empty object allocated here is never used.

for (const key in defaultProps) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched the Object.keys for a for ... in loop, which is more efficient.

if (Object.prototype.hasOwnProperty.call(defaultProps, key)) {
const propName = key as keyof T;

if (!slotProps || !Object.keys(slotProps)) {
// Reduce the iteration if the slot props is empty
output[propName] = defaultSlotProps;
Comment on lines -28 to -30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

!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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} else if (!defaultSlotProps || !Object.keys(defaultSlotProps)) {
// Reduce the iteration if the default slot props is empty
output[propName] = slotProps;
} else {
output[propName] = { ...slotProps };
Object.keys(defaultSlotProps).forEach((slotPropName) => {
(output[propName] as Record<string, unknown>)[slotPropName] = resolveProps(
(defaultSlotProps as Record<string, any>)[slotPropName],
(slotProps as Record<string, any>)[slotPropName],
);
});
if (propName === 'components' || propName === 'slots') {
Copy link
Contributor Author

@romgrk romgrk Jul 30, 2024

Choose a reason for hiding this comment

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

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.

output[propName] = {
...(defaultProps[propName] as any),
...(output[propName] as any),
};
} else if (propName === 'componentsProps' || propName === 'slotProps') {
const defaultSlotProps = defaultProps[propName] as T[keyof T] | undefined;
const slotProps = props[propName] as {} as T[keyof T] | undefined;

if (!slotProps) {
output[propName] = defaultSlotProps || ({} as T[keyof T]);
} else if (!defaultSlotProps) {
output[propName] = slotProps;
} else {
output[propName] = { ...slotProps };

for (const slotKey in defaultSlotProps) {
if (Object.prototype.hasOwnProperty.call(defaultSlotProps, slotKey)) {
const slotPropName = slotKey;
(output[propName] as Record<string, unknown>)[slotPropName] = resolveProps(
(defaultSlotProps as Record<string, any>)[slotPropName],
(slotProps as Record<string, any>)[slotPropName],
);
}
}
}
} else if (output[propName] === undefined) {
output[propName] = defaultProps[propName];
}
} else if (output[propName] === undefined) {
output[propName] = defaultProps[propName];
}
});
}

return output;
}