[typescript] Add component field to *Props types
#38084
Conversation
michaldudak
added
core
Netlify deploy previewhttps://deploy-preview-38084--material-ui.netlify.app/ Bundle size report |
| @@ -18,6 +18,6 @@ declare const Box: OverridableComponent<BoxTypeMap<{}, 'div', MaterialTheme>>; | |||
| export type BoxProps< | |||
| D extends React.ElementType = BoxTypeMap['defaultComponent'], | |||
| P = {}, | |||
| > = OverrideProps<BoxTypeMap<P, D, MaterialTheme>, D>; | |||
| > = OverrideProps<BoxTypeMap<P, D, MaterialTheme>, D> & { component?: D }; | |||
There was a problem hiding this comment.
@michaldudak after adding {component?:D} here - test from Stack.spec.tsx file is failing. here is the error from CI
There was a problem hiding this comment.
Box already has the component defined in its TypeMap (packages\mui-system\src\Box\Box.d.ts), so it's not necessary to add it here.
There was a problem hiding this comment.
okay fair but when Box is replaced with Button - issue still persists. here is the codesandbox link : https://codesandbox.io/s/exciting-cloud-r4qvcm?file=/Demo.tsx. maybe underlying issue is different
There was a problem hiding this comment.
Ah, right. In this case, when we don't provide a type parameter to the ButtonProps, it defaults to button, and make the whole type too strict to be passed to the Stack's component.
Defining the *Props with & { component?: React.ElementType } instead would help here, but it would allow writing <Button<'span'> component='div' />. I think we can have such a compromise.
There was a problem hiding this comment.
I just committed this change, let's see how the CI will react.
There was a problem hiding this comment.
CI is green, shall I go ahead and add tests?
| <Card | ||
| component="a" | ||
| href="test" | ||
| onClick={(event) => { |
There was a problem hiding this comment.
Why was this assertion removed?
There was a problem hiding this comment.
@michaldudak after adding { component?: React.ElementType } to components, are types of event handlers expected to be inherited from component?
for example in below code - is type of event expected to be React.MouseEvent<HTMLDivElement, MouseEvent> or React.MouseEvent<HTMLAnchorElement, MouseEvent>. Right now event type is React.MouseEvent<HTMLDivElement, MouseEvent>
<Card
component='a'
onClick={(event) => {
}}
/>
There was a problem hiding this comment.
Yes - the *Props types are not used when you instantiate a component (OverridableComponent does not use *Props), so there should be no changes in this matter.
In case of the Card, this isn't true, though - its TypeMap uses PaperProps, which likely causes this problem.
There was a problem hiding this comment.
@sai6855 I'm working on a fix for this.
| React.createElement<CardHeaderProps<typeof CustomComponent>>(CardHeader, { | ||
| // @ts-expect-error | ||
| React.createElement<CardHeaderProps<DefaultComponent, ComponentProp>>(CardHeader, { | ||
| // This test shouldn't fail but does; stringProp & numberProp are required props of CustomComponent |
There was a problem hiding this comment.
It's been like this even before #35924 - this is just restoring the original behavior.
As for why it's like this in the first place - I'm investigating it.
There was a problem hiding this comment.
It looks like the test is incorrect: the type parameter was DefaultComponent while the component prop was set to something else. It is expected to fail.
michaldudak
changed the title
component to OverrideProps
michaldudak
force-pushed
the
revert-overrideprops
branch
from
0da0d4c to
11016d3
Compare
michaldudak
changed the title
component to OverridePropscomponent field to *Props types
|
@mnajdova, I addressed your remarks. |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
There was a problem hiding this comment.
Tests look great, before merging please add descriptions next to each // @ts-expect-error
|
@sai6855 Thanks for your help with this! |
This is a different implementation of the fix proposed in #35924, which was reverted in #38150, as it caused some problems.
Instead of adding the
{ component?: D }toOverrideProps,{ component?: React.ElementType }is added to each individual*Propstype. This solution doesn't cause problems as*Propsaren't used where a component is instantiated (they are not a part ofOverridableComponentdefinition), unlikeOverrideProps.OverridableComponentalready has a definition for thecomponentprop and it's used to determine if a component uses its default root, or a customized one. Adding yet another definition ofcomponentto it turned out to cause issues.Tests written in #35924 were copied to this PR. Also, tests verifying the regressions reported after #35924 was released were also added to the PR.