Skip to content
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] Remove few more React.ReactElement<any> types #44290

Merged
merged 19 commits into from
Nov 28, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 1, 2024

Part of: #42380

All of remaining instances of React.ReactElement<any> are in same file, i'll create a seprate PR for it.

image

@sai6855 sai6855 marked this pull request as draft November 1, 2024 07:49
@sai6855 sai6855 added typescript package: material-ui Specific to @mui/material labels Nov 1, 2024
@mui-bot
Copy link

mui-bot commented Nov 1, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d73a827

@sai6855 sai6855 changed the title [material-ui] Remove React.ReactElement<any> types [material-ui] Remove few more React.ReactElement<any> types Nov 1, 2024
@sai6855 sai6855 marked this pull request as ready for review November 1, 2024 13:25
@sai6855 sai6855 requested a review from DiegoAndai November 1, 2024 13:25
Copy link
Member

@DiegoAndai DiegoAndai left a 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 @sai6855!

@@ -7,7 +7,7 @@ import Typography from '@mui/material/Typography';
import { useSpring, animated } from '@react-spring/web';

interface FadeProps {
children: React.ReactElement<any>;
children: React.JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change of these should be

-children: React.ReactElement<any>;
+children: React.ReactElement<unknown>;

Why React.JSX.Element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript is throwing error in below line, if React.ReactElement<unknown> is used as if doesn't have onClick type, hence i went with React.JSX.Element

{React.cloneElement(children, { onClick })}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should do React.ReactElement<{ onClick: React.MouseEventHandler}>, because we require the children to support this prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, i had posted reason in above comment why i didn't went with React.ReactElement<{ onClick: React.MouseEventHandler}> somehow it didn't get saved.

Anyway, if i went with React.ReactElement<{onClick?:React.MouseEventHandler}> ts throwing below error here (on line 79)

slotProps={{
backdrop: {
TransitionComponent: Fade,
},
}}

Types of property 'props' are incompatible. Type 'unknown' is not assignable to type '{ onClick?: MouseEventHandler<Element> | undefined; }'.

I wasn't sure how to fix above error hence went with React.JSX.Element

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, at this point, I think it's better to keep React.ReactElement<any> in this file (which is virtually the same as having React.JSX.Element), so let's go with React.ReactElement<any> to make it explicit.

const createHandleSynthetic = (handlerName: string) => (event: React.SyntheticEvent) => {
type ReactSyntheticEventName = ClickAwayMouseEventHandler | ClickAwayTouchEventHandler;

const createHandleSynthetic = (handlerName: ReactSyntheticEventName) => (event: any) => {
Copy link
Contributor Author

@sai6855 sai6855 Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event is tricky to type here, we need to narrow type of event based on handlerName

For example, if handlerName is OnTouchStart type of event should be React.TouchEvent, if handlerName is onClick type of event should be React.MouseEvent.

I'm not sure if above functionlaity can be achived without adding some runtime code, i'm not sure if it's worth here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about React.UIEvent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even React.UIEvent isn't working.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's revert to children: React.ReactElement<any>; in this one (all changes in this file). We can fix it in the future if we have a report from a user.

@@ -7,7 +7,7 @@ import Typography from '@mui/material/Typography';
import { useSpring, animated } from '@react-spring/web';

interface FadeProps {
children: React.ReactElement<any>;
children: React.JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, at this point, I think it's better to keep React.ReactElement<any> in this file (which is virtually the same as having React.JSX.Element), so let's go with React.ReactElement<any> to make it explicit.

@@ -5,7 +5,7 @@ declare class ServerStyleSheets {
constructor(options?: object);
collect(children: React.ReactNode, options?: object): React.ReactElement<StylesProviderProps>;
toString(): string;
getStyleElement(props?: object): React.ReactElement<any>;
getStyleElement(props?: object): React.JSX.Element;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep React.ReactElement<any> here as well

Copy link
Contributor Author

@sai6855 sai6855 Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here 451e23d and 188a956

const createHandleSynthetic = (handlerName: string) => (event: React.SyntheticEvent) => {
type ReactSyntheticEventName = ClickAwayMouseEventHandler | ClickAwayTouchEventHandler;

const createHandleSynthetic = (handlerName: ReactSyntheticEventName) => (event: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about React.UIEvent?

@sai6855 sai6855 requested a review from DiegoAndai November 27, 2024 05:37
@DiegoAndai DiegoAndai merged commit e1479de into mui:master Nov 28, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants