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

[RFC] Remove sx-like props #41539

Closed
siriwatknp opened this issue Mar 18, 2024 · 10 comments
Closed

[RFC] Remove sx-like props #41539

siriwatknp opened this issue Mar 18, 2024 · 10 comments
Labels
deprecation New deprecation message package: material-ui Specific to @mui/material RFC Request For Comments v6.x

Comments

@siriwatknp
Copy link
Member

siriwatknp commented Mar 18, 2024

Context

Example of sx-like props:

<Typography mx={2}>
<Box display="flex">

These components currently support sx-like props:

  • Typography
  • Link
  • Box
  • Grid
  • Stack

Problems

  • it is the same feature as sx prop. we should not have multiple APIs that do the same thing.
  • create inconsistency in the DX. I have seen a lot of projects mixing between sx prop and sx-like props. Copy-paste from one to another is frustrated.
  • create confusion for newcomers (why only these components can do this pattern?).
  • a burden for maintainers. imagine there is a migration and we have to create a codemod that supports two patterns.
  • a huge work to make Pigment CSS to support sx-like props

Proposal

Remove the sx-like props from all of the components and always use sx prop on Material UI component. The sx prop already indicates that there is something specific to the library, not React or HTML things.

There will be no deprecation since there are not a lot of components in the list. All of them will be updated in a single PR.

@siriwatknp siriwatknp added breaking change package: material-ui Specific to @mui/material v6.x RFC Request For Comments labels Mar 18, 2024
@mwskwong
Copy link
Contributor

mwskwong commented Mar 19, 2024

Previously, I have suggested the exact opposite, which is to allow other components to make use of sx-like props #37677. But then some one pointed out this can be hard to do on components that already have props of the same name. So I believe this RFC is the 2nd best option for consistency.

However, if we are going towards this route, I think MUI should provide a mergeSx() util out of the box, since the usage of it will just increase when this change happened.

@siriwatknp
Copy link
Member Author

What do you mean by mergeSx()?

@mwskwong
Copy link
Contributor

What do you mean by mergeSx()?

Basically the logic of https://mui.com/system/getting-started/the-sx-prop/#passing-the-sx-prop.
It is not super complicated by itself, but it is complex enough to not to repeat this on multiple components.

That's why a util function like this exists: https://github.com/RobinTail/merge-sx

@Rishi556
Copy link
Contributor

As long as there's a codemod that can be used to convert all of our existing like props, I think this is a good move.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 27, 2024

Removing those makes sense to me. I personally never use them, and they will be hard to support for Pigment CSS. Having one prop dedicated to styles makes sense, e.g. className prop for utility classes systems. It also reduces the decision fatigue for developers.

On the execution, it seems that it should be deprecated in Material UI v6, without support for Pigment CSS and then removed in Material v7. It feels too much of a pain/brutal to remove them without a deprecation phase and even with codemods.

@oliviertassinari oliviertassinari added deprecation New deprecation message and removed breaking change labels Mar 27, 2024
@siriwatknp
Copy link
Member Author

On the execution, it seems that it should be deprecated in Material UI v6, without support for Pigment CSS and then removed in Material v7. It feels too much of a pain/brutal to remove them without a deprecation phase and even with codemods.

I think it's better to do breaking change than deprecations given that the sx-like props are in some components. It would reduce a lot of complexity on our side.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 1, 2024

@siriwatknp We are paying for the complexity today, and we won't do the effort to make it work Pigment CSS. As much as it would be great to just remove stuff in one major, I don't think that it's how we should operate, as a user, it would make it feel that we are disconnected from real-life projects and reinforce that "when you use something by MUI, you subscribe to costly migrations". Instead, I think that spaning changes in two majors and make more frequent majors would be great.


Basically the logic of https://mui.com/system/getting-started/the-sx-prop/#passing-the-sx-prop.
It is not super complicated by itself, but it is complex enough to not to repeat this on multiple components.
That's why a util function like this exists: https://github.com/RobinTail/merge-sx

@mwskwong I have found the issue in question: #37677. I'm moving the conversation there 👍

@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2024

A relevant work on this https://primer.style/react/system-props

System props are deprecated in all components except Box and Text. Please use the sx prop instead.

For context primer/react#1336. We could copy their codemod.

@siriwatknp
Copy link
Member Author

siriwatknp commented May 27, 2024

The team agreed to do this so I'm closing this issue. Follow #42259 for the next step.

@oliviertassinari

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation New deprecation message package: material-ui Specific to @mui/material RFC Request For Comments v6.x
Projects
None yet
Development

No branches or pull requests

4 participants