-
-
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
[material-ui] Change React.ReactElement<any> to React.ReactElement<unknown> #43402
Conversation
Netlify deploy previewhttps://deploy-preview-43402--material-ui.netlify.app/ Bundle size report |
@@ -91,7 +91,7 @@ function throwMissingPropError(field: string): never { | |||
* the root component. | |||
*/ | |||
export function testClassName( | |||
element: React.ReactElement<any>, | |||
element: React.ReactElement<HTMLElement & { 'data-testid'?: string }>, |
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.
All changes of any
to unknown
(some were changed to HTMLElement) in this PR are straight forward, except for changes in this file.
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 explain the criteria you used to choose the type of element
in the different functions? I'm trying to understand the impact it will have on the describeConformance
usage. Will elements passed to testClassName
without the 'data-tested'
prop throw a type error? Where would that error show?
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.
You are right, calling testClassName without passing data-testid
is throwing type error.
This is tricky to fix, data-testid
is added to element
as below.
React.cloneElement(element, { className, 'data-testid': testId }), |
I think we have 2 options to fix
- add
data-testid
type to argument as above, but it throws type error when consumers of this function doesn't adddata-testid
type - Keep
element
argument type asReact.ReactElement<unknown>
and type castelement
type near it's usage as shown below.
React.cloneElement(element + as React.ReactElement<unknown & {[data-testid]:string}> , { className, 'data-testid': testId }),
By going with option 2 , we can keep types of consumers of this function to be flexible. what do you think?
What i did for now is, i've reverted all changes in this file here, what ever option we choose i'll create a seprate PR as this PR is alread too big and i don't want this descion to block this PR from merging.
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.
Let's open another PR and discuss it there 👌🏼
I think we can create a type that is the same as ReactElement but supports data-
attributes
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 taking this @sai6855, I left some questions
@@ -13,7 +13,7 @@ interface Props { | |||
* You won't need it on your project. | |||
*/ | |||
window?: () => Window; | |||
children?: React.ReactElement<any>; | |||
children?: React.ReactElement<unknown & { elevation?: number }>; |
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.
Why is & { elevation?: number }
needed?
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.
Here elevation
prop is added to children, since unknown
type doesn't have elevation
type, i explicitly added. I'm not entirely sure if this is optimal, but please let me know if you have better suggestion.
elevation: trigger ? 4 : 0, |
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.
I think we can have only the elevation prop:
children?: React.ReactElement<unknown & { elevation?: number }>; | |
children?: React.ReactElement<{ elevation?: number }>; |
As unknown is absorbed in intersections:
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.
fixed here 3ff9828
@@ -306,7 +306,7 @@ function render( | |||
traceSync('forceUpdate', () => | |||
testingLibraryRenderResult.rerender( | |||
React.cloneElement(element, { | |||
'data-force-update': String(Math.random()), | |||
['data-force-update' as string]: String(Math.random()), |
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.
Why is as string
required?
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.
here element
type is unknown
, since unknown doesn't have data-force-update
type, i've explicitly marked type as as string
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.
But if I understand correctly, this is casting the key, not the value 🤔
What's the error thrown without the cast?
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.
Instead of casting, i've tried to fix type error properly here =>09f5298
@@ -82,7 +82,7 @@ export interface CreateCssVarsProviderResult< | |||
disableStyleSheetGeneration?: boolean; | |||
} | |||
>, | |||
) => React.ReactElement<any>; | |||
) => React.ReactElement<unknown>; |
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.
What do you think should this type be @siriwatknp? It's the return value of CssVarsProvider
, we should have it's props somewhere, right?
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.
packages/mui-styles/src/ServerStyleSheets/ServerStyleSheets.d.ts
Outdated
Show resolved
Hide resolved
@@ -8,4 +8,4 @@ export interface GlobalStylesProps<Theme = {}> { | |||
|
|||
export default function GlobalStyles<Theme = {}>( | |||
props: GlobalStylesProps<Theme>, | |||
): React.ReactElement<any>; | |||
): React.ReactElement<unknown>; |
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.
@siriwatknp what do you think this type should be?
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.
@@ -91,7 +91,7 @@ function throwMissingPropError(field: string): never { | |||
* the root component. | |||
*/ | |||
export function testClassName( | |||
element: React.ReactElement<any>, | |||
element: React.ReactElement<HTMLElement & { 'data-testid'?: string }>, |
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 explain the criteria you used to choose the type of element
in the different functions? I'm trying to understand the impact it will have on the describeConformance
usage. Will elements passed to testClassName
without the 'data-tested'
prop throw a type error? Where would that error show?
4511fd4
to
09f5298
Compare
@DiegoAndai updated PR based on #43402 (comment) #43402 (comment) it is ready for final review |
@@ -25,7 +25,7 @@ export interface GlobalStylesProps { | |||
styles: SCGlobalStylesProps<Theme>['styles']; | |||
} | |||
|
|||
function GlobalStyles(props: GlobalStylesProps): React.ReactElement<any> { | |||
function GlobalStyles(props: GlobalStylesProps): React.ReactElement<unknown> { |
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.
Seems like we should change this one to React.JSX.Element
as well.
@@ -25,7 +25,7 @@ export interface GlobalStylesProps { | |||
styles: EmGlobalStylesProps<Theme>['styles']; | |||
} | |||
|
|||
function GlobalStyles(props: GlobalStylesProps): React.ReactElement<any> { | |||
function GlobalStyles(props: GlobalStylesProps): React.ReactElement<unknown> { |
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.
Seems like we should change this one to React.JSX.Element
as well.
@@ -8,4 +8,4 @@ export interface GlobalStylesProps<Theme extends object = {}> { | |||
|
|||
export default function Global<Theme extends object = {}>( | |||
props: GlobalStylesProps<Theme>, | |||
): React.ReactElement<any>; | |||
): React.ReactElement<unknown>; |
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.
Seems like we should change this one to React.JSX.Element
as well.
Thanks @sai6855! |
Part of: #42380
follow up on #43358 (review)
As of now, there are 7 instances of React.ReactElement left to be migrated except for
base/, joy/
folders. Remaining chnages are not straight forward changes from any to unknown, as those requires few more type changes. i'll open a new PR for left over instances