Skip to content

[system] Fix changed types between 5.14.0 and 5.14.1 #39288

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

Closed
wants to merge 2 commits into from
Closed

[system] Fix changed types between 5.14.0 and 5.14.1 #39288

wants to merge 2 commits into from

Conversation

gitstart
Copy link
Contributor

@gitstart gitstart commented Oct 3, 2023

Description

Update the styled component typing to include component prop typing.

closes #38626


This code was written and reviewed by GitStart Community. Growing great engineers, one PR at a time.

@mui-bot
Copy link

mui-bot commented Oct 3, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 15aec35

@danilo-leal danilo-leal changed the title MUI-38626 - [material] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled [material-ui] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled Oct 3, 2023
@danilo-leal danilo-leal added package: material-ui Specific to @mui/material package: styled-engine Specific to @mui/styled-engine labels Oct 3, 2023
@danilo-leal danilo-leal changed the title [material-ui] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled [system] Fix changed types between 5.14.0 and 5.14.1 Oct 3, 2023
@danilo-leal danilo-leal added package: system Specific to @mui/system and removed package: material-ui Specific to @mui/material labels Oct 3, 2023
@gitstart gitstart marked this pull request as ready for review October 6, 2023 15:28
@gitstart
Copy link
Contributor Author

gitstart commented Oct 6, 2023

@DiegoAndai this PR is ready for review

@zannager zannager requested a review from DiegoAndai October 6, 2023 16:52
@DiegoAndai
Copy link
Member

Hey @gitstart, thanks for working on this. Could you explain in depth how these changes fix the issue? It would make reviewing the typing change way easier.

@gitstart
Copy link
Contributor Author

gitstart commented Oct 10, 2023

Hey @gitstart, thanks for working on this. Could you explain in depth how these changes fix the issue? It would make reviewing the typing change way easier.

@DiegoAndai Based on the details provided in the issue, it seems that the newly introduced component props in this location were preventing the styled component from accessing the correct typings as seen here. We suspect that the issue might have been related to this definition. Given that the generic Props type wasn't utilized, it would default to any type as seen here.

To address this, we decided to explicitly include the component typing in the type definition. However, there's still some uncertainty regarding why this definition wasn't inferred as React.ComponentProps. Ideally, it should have been sufficient to account for the introduction of the component props.

@DiegoAndai
Copy link
Member

@gitstart, thanks for the explanation. Sadly, I don't have access to any of the links 😅

Seems like these are under https://github.com/GitStartHQ/, so maybe I don't have access to that. May I ask you to update it to reference this repo? If that's not possible, adding code snippets is another option.

About this change:

   <
     C extends React.JSXElementConstructor<React.ComponentProps<C>>,
-    ForwardedProps extends keyof React.ComponentProps<C> = keyof React.ComponentProps<C>,
+    RootComponent extends React.ElementType = React.ElementType,
+    ForwardedProps extends keyof OverriddableForwardProps<
+      C,
+      RootComponent
+    > = keyof OverriddableForwardProps<C, RootComponent>,
   >(

This means that now the third argument is the forwarded props, right? Why is that? Maybe that's the cause of our issue

@gitstart
Copy link
Contributor Author

@gitstart, thanks for the explanation. Sadly, I don't have access to any of the links 😅

Seems like these are under https://github.com/GitStartHQ/, so maybe I don't have access to that. May I ask you to update it to reference this repo? If that's not possible, adding code snippets is another option.

About this change:

   <
     C extends React.JSXElementConstructor<React.ComponentProps<C>>,
-    ForwardedProps extends keyof React.ComponentProps<C> = keyof React.ComponentProps<C>,
+    RootComponent extends React.ElementType = React.ElementType,
+    ForwardedProps extends keyof OverriddableForwardProps<
+      C,
+      RootComponent
+    > = keyof OverriddableForwardProps<C, RootComponent>,
   >(

This means that now the third argument is the forwarded props, right? Why is that? Maybe that's the cause of our issue

@DiegoAndai The links on the comment have been updated, apologies for the oversight.

@DiegoAndai
Copy link
Member

@gitstart so what do you think about this:

   <
     C extends React.JSXElementConstructor<React.ComponentProps<C>>,
-    ForwardedProps extends keyof React.ComponentProps<C> = keyof React.ComponentProps<C>,
+    RootComponent extends React.ElementType = React.ElementType,
+    ForwardedProps extends keyof OverriddableForwardProps<
+      C,
+      RootComponent
+    > = keyof OverriddableForwardProps<C, RootComponent>,
   >(

From what I understand, this means that now the third argument is the forwarded props, right? Why is that? Maybe if we figure out why, we could pinpoint the root cause of the issue.

@gitstart
Copy link
Contributor Author

@gitstart so what do you think about this:

   <
     C extends React.JSXElementConstructor<React.ComponentProps<C>>,
-    ForwardedProps extends keyof React.ComponentProps<C> = keyof React.ComponentProps<C>,
+    RootComponent extends React.ElementType = React.ElementType,
+    ForwardedProps extends keyof OverriddableForwardProps<
+      C,
+      RootComponent
+    > = keyof OverriddableForwardProps<C, RootComponent>,
   >(

From what I understand, this means that now the third argument is the forwarded props, right? Why is that? Maybe if we figure out why, we could pinpoint the root cause of the issue.

@DiegoAndai The reason why forwardProps is the third argument is that the second argument is used to infer the type of the component props, which is the RootComponent. The RootComponent is used for the typing of ForwardedProps and also for CreateStyledComponent typing.

@DiegoAndai
Copy link
Member

DiegoAndai commented Oct 25, 2023

The reason why forwardProps is the third argument is that the second argument is used to infer the type of the component props, which is the RootComponent. The RootComponent is used for the typing of ForwardedProps and also for CreateStyledComponent typing.

@gitstart then my question is why this change made the RootComponent become the second argument? I tested and removing & { component?: React.ElementType; } made this bug disappear. So, is the RootComponent argument supposed to be there? Or should we fix it because it shouldn't? If it is correct for RootComponent to be the third argument, we should probably update all three types on CreateMUIStyled that use forwarded props as the second argument, as that should be the third, right?

@GitStartHQ GitStartHQ closed this by deleting the head repository Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine Specific to @mui/styled-engine package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled
5 participants