material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

Published 20 hours ago • verified publisher icon mui

[core] Remove unnecessary conditional around `.muiName = `

Open Janpot opened this issue 1 year ago • 8 comments

This was added in #42001. Why?

Janpot avatar Oct 11 '24 13:10 Janpot

Netlify deploy preview

https://deploy-preview-44071--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against 14761c3ea227e7ee6164bee76055c354378480c7

mui-bot avatar Oct 11 '24 13:10 mui-bot

It could be a use case for https://github.com/mui/pigment-css/issues/84.

I have tried to apply this change with https://github.com/mui/material-ui/tree/4bcf32bbb8db43c5fe8c52d300116af00aae40d1/examples/material-ui-pigment-css-vite-ts (and found #44074 along the way), it seems to work fine. A big 💯 to move in this direction. If this code branche is needed, then muiName isn't applied, then it's broken, so in any cases, we can't have this code branch.

oliviertassinari avatar Oct 11 '24 23:10 oliviertassinari

Yep, also found that adding .displayName to a styled component breaks pigment. Still investigating.

Janpot avatar Oct 12 '24 07:10 Janpot

I can confirm that it was added during the migration of Pigment CSS, because otherwise it breaks Pigment CSS. @brijeshb42 can likely share more info on why (or @siriwatknp as he was mainly driving the migration).

mnajdova avatar Oct 16 '24 08:10 mnajdova

It is fine to remove now since we added a fix in the Pigment transform itself.

brijeshb42 avatar Oct 16 '24 09:10 brijeshb42

It is fine to remove now since we added a fix in the Pigment transform itself.

Does the fix apply to .displayName as well? It seems to break pigment in https://github.com/mui/material-ui/pull/44067

Janpot avatar Oct 16 '24 09:10 Janpot

On inspecting the code, I see that we only handle muiName and propTypes. We'll also need to add check for displayName.

brijeshb42 avatar Oct 16 '24 09:10 brijeshb42

There is also defaultProps, and Toolpad is also attaching a static (Symbol) property to the component. I've seen other tools rely on similar patterns. Would it be possible to support just any static property on a component?

Janpot avatar Oct 16 '24 10:10 Janpot

Sorry for the delay, this PR got buried. @brijeshb42 can you confirm if we can merge this one?

DiegoAndai avatar Dec 19 '24 19:12 DiegoAndai

We can merge it as long as the example apps (during CI) are building correctly after rebasing.

brijeshb42 avatar Dec 20 '24 09:12 brijeshb42

Nice

SCR-20241220-oppi

https://tools-public.mui.com/prod/pages/bundleSizes?circleCIBuildNumber=780755&baseRef=master&baseCommit=cef5e6cd7af197c835acb3b1a37733f7e82902f2&prNumber=44071

oliviertassinari avatar Dec 20 '24 15:12 oliviertassinari