-
-
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
[Card] Fix TypeScript not recognizing "component" prop #20093
Conversation
…graphyProps` contains a `component` prop (introduced on commit b6ff4f5 removing component as explicit prop).
Details of bundle changes.Comparing: 5fc9926...968b06b Details of page changes
|
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.
Thanks for working on this!
This is fixing symptoms but not the underlying cause. The fix should happen at TypographyProps
.
Thanks for the quick reply, @eps1lon 🙂
Agree. I was blindsided by my own build error 😅 Done! |
@eps1lon I added the fix to Restoring the prop, still seems to comply with the desired usage that motivated the change. Thoughts? @oliviertassinari @fyodore82 @eps1lon Like so: export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'> {
props: P & {
// ...etc
component?: D;
};
defaultComponent: D;
classKey: TypographyClassKey;
} It used to be... export interface TypographyProps
extends StandardProps<React.HTMLAttributes<HTMLElement>, TypographyClassKey> {
// ...etc
component?: React.ElementType<React.HTMLAttributes<HTMLElement>>;
} Wondering also if there are concerns stopping this from getting processed and how can we help? After all this was an unintended breaking change. For the other affected members of the community who have shown some discomfort, whilst this is resolved unless you're specifically trying to use some of the latest release features, you may change you package.json to use exactly 4.7.2 (i.e. P.S. I think there is one or more raise condition(s) with the (automated) tests. I see them fail and succeed unexpectedly on this PR which doesn't involve any of the failed test. Feel free to see the commit history and inspect the failures. |
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.
Could you add test cases to 1. prevent regressions in the future, 2. make sure it behaves as intended? Thanks
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.
… (title & subheader)
…t' into fix-nested-typography-prop-component # Conflicts: # packages/material-ui/src/CardHeader/CardHeader.spec.tsx # packages/material-ui/src/ListItemText/ListItemText.spec.tsx
Apologies, guys. I sort messed up the rebase. So opened a new clean PR with this work. |
For example, CardHeader breaks TypeScript compilation when
titleTypographyProps
orsubheaderTypographyProps
contains acomponent
prop. In version v4.7.2, the below used to work:Somewhere after (4.8.3 o 4.9.5), TypeScript compilation for consumer apps (verified on TypeScript 3.8.3) broke due to a PR removing the component as an explicit prop of Typography.
Checking the release notes I didn't see this noted as a breaking change so I suspect this wasn't quite the idea.