-
-
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
[system] Fix composeClasses v6 behavior change #43537
Conversation
Netlify deploy previewhttps://deploy-preview-43537--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
85f2239
to
cd05f43
Compare
cd05f43
to
bcffcbb
Compare
bcffcbb
to
7fdbf72
Compare
By taking inspiration from https://github.com/lukeed/clsx/blob/master/src/lite.js the performance could be improved even more by writing the function like so: function composeClasses(
slots,
getUtilityClass,
classes = undefined,
) {
let classValue, output = {}, slot, i = 0, buffer = '', value, slotName;
for (slotName in slots) {
slot = slots[slotName]
for (; i < slot.length; i ++) {
if (value = slot[i]) {
buffer += (buffer && ' ') + getUtilityClass(value);
if (classes && (classValue = classes[value])) {
buffer += ' ' + classValue;
}
}
}
output[slotName] = buffer;
}
return output;
} I ran some tests over at https://jsbench.me/, which will run the code a lot more times than jsben.ch and it showed 25% improvement over code currently in v6. If such code is not up for code convention of mui, changing |
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 changes proposed by @HermanBilous also LGTM, caching the result of classes[value]
can be good.
+1 to explore those proposed changes, if we can get an extra performance gain, with a low bundler size overhead, we should grab it. I don't have the bandwidth to carry this PR further, @mnajdova you merged the initial PR, handing this over to you. |
@oliviertassinari Should me merge this and the proposed changes can be taken up in a follow up ? |
Yes, it'd be great if we could merge this sooner rather than later. |
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.
I am merging as this is a regression. @HermanBilous would you like to open a PR with the proposed changes as a follow up?
@HermanBilous I have tried your suggestion at https://jsben.ch/UE7FD, but it seems to be more or less the same: ![]() I have also tried https://github.blog/news-insights/product-news/openai-o1-in-github-copilot/#optimize-complex-algorithms-with-advanced-reasoning, but GitHub Copilot gave me crap. Maybe with o1, it will get better at it. |
Fix the behavior change introduced in #43363. I worked on this PR because I was trying a button customize in the store and got frustrated in the DX to find the class names applied:
The performance win stays comparable as before https://jsben.ch/S3LCV
A related design principle: https://www.usertesting.com/blog/gestalt-principles#principle-#3:-proximity.