-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Fix 3884 text styles not applied correctly #5253
Fix 3884 text styles not applied correctly #5253
Conversation
chore: update branch to latest from main
🦋 Changeset detectedLatest commit: f0884f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/chakra-ui/chakra-ui-storybook/2uWuqes2Vj6JHMTQ44anKn8GZCHN |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f0884f1:
|
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.
Thank you @primos63 for this PR ✨
I really like the improvements you did to the code structure and readability.
Kindly see my comments in the code to see the performance impact.
I think we may need a re-architecture of the styled-system that's focused heavily on performance. When we do that, we will include the idea behind your solution.
We are not able to move forward with this PR. Feel free to reach out to me to discuss this more!
Thanks for understanding
const computedCSS = stylePrecedence.reduce((accStyle: any, style: any) => { | ||
return mergeWith(accStyle, css(style)(theme)) | ||
}, {}) |
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.
The mergeWith
function causes to increase the runtime complexity to increase from O(n) to O(n^2). Sadly this is in the styled-system
algorithm hot path, which will be called by every chakra component on every render.
A very simple loop test gave us the following average time in ms for a run with 10000 iterations:
PR solution: 0.045435ms
Current main branch: 0.02831ms
The large chunk of the performance regression is caused by running mergeWith
and css
function in a loop, compared to the current approach that uses Object.assign
and runs the css
function once.
Closes #3884
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information