-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Changes from all commits
f289863
c57be7c
2709aa1
ee3f5cd
a149f07
72c79d0
c0ac96e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting |
||
const slotProps = props[propName] as {} as T[keyof T]; | ||
output[propName] = {} as T[keyof T]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched the |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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.
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