-
-
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
[Grid] Remove unnecessary type exports #26187
Conversation
We have these in all sorts of places and just keep them for ergonomic purposes. Especially if the actual props interface requires type arguments. Then it's always a bit much boilerplate to do I don't see a point doing this right now considering it's a breaking change. |
@@ -23,8 +23,10 @@ type GridJustification = | |||
| 'space-around' | |||
| 'space-evenly'; | |||
|
|||
type Direction = Exclude<GridProps['direction'], undefined>; |
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.
This kind of prooves my point. Now you need to know Exclude
and then create this helper type when you could just re-use the existing one.
What's really important here is the difference between adding an export and removing it. As a purely technical decision this would be sound. But as a breaking change it doesn't add any value beyond technical purity. |
The breaking change was motivated by the pain raised in #19572. I have proposed it because the Grid seemed to be the only component that exports type for some of its props. Back then I have assumed that it was completely arbitrary and harms the developer experience as once learned, it can't be reemployed on the other components. Happy with no matter what the outcome is. It seems to be a low-cost but only low-win breaking change. I think that hearing others' points of view would help, the tradeoff seems to be around the DX. |
These are all just unfounded. Like, what are we gaining here? Please keep this a constructive discussion and don't just make things up again in hindsight. It is a breaking change. Period. What the cost is, is not for you to declare. What the win is? Was not answered in the PR. We're just getting into a mood where we make breaking changes for their own sake. |
I think that it would solve #19572. In this case, the user was extending the Grid, he had imported individual types to do so. Then, he needed to extend the Typography, but couldn't do it the same way he had extended the Grid because we don't export individual types. By removing the types in the Grid, we force him to find a scalable solution, from the beginning, avoiding the frustration to figure one solution out until to realize after that he has to drop it because it allows works in a niche case. It seems to be a low-win, avoiding a small foot-gun.
Some breaking changes impact 100% of the users (e.g. drop JSS), others 2% (e.g. remove RTL support). Here, it seems a low-cost change. "low-cost" as impacting a low % of the user-base (<1% ?). It's a guess, we don't have a perfect level of information. cc @mui-org/core for others' perspectives, on what we should do with this public API. Maybe we can run a vote 🚀 for moving forward and 👀 for giving up on the BC on the PR's description? |
Why is this being voted on? Why are you desperately trying to force overruling me? |
It's a closed issue we already solved. Am I being kept out of critical communication or what is going on right now? |
I don't see anyone ever reported something related to these typings. I think we can skip this BC, I don't see value in it, to be honest. |
Ok, so it seems that we should close, we have one voice really against (Sebastian), one leaning against (Marija), one leaning in favor (Olivier). @m4theushw thanks for looking into it! |
Closing this as it brings low value to users. |
Breaking changes
[Grid] Removed the export of the types
GridDirection
,GridSpacing
,GridWrap
andGridSize
. UseGridProps
to replace them:Part of #20012