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.

Sign up for GitHub

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

Jump to bottom

Add missing PropsWithRef type to fix #4293 #4333

Closed
wants to merge 1 commit into from
Closed

Add missing PropsWithRef type to fix #4293 #4333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion compat/src/index.d.ts
View file
Open in desktop
Expand Up @@ -178,7 +178,7 @@ declare namespace React {
C extends ComponentType<any> | keyof JSXInternal.IntrinsicElements
> = C extends new (props: infer P) => Component<any, any>
? PropsWithoutRef<P> & RefAttributes<InstanceType<C>>
: ComponentProps<C>;
: PropsWithRef<ComponentProps<C>>;
Copy link
Member

Choose a reason for hiding this comment

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

Bear with me as I'm doing this from my phone, but I don't see PropsWithRef defined anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

Ah - that's totally my miss. I'm still trying to figure out problems with other MUI components on another branch and didn't see that I'd missed copying the type here. Unfortunately, after copying, it doesn't appear to resolve the issue. I'll keep digging. Sorry for that!

Copy link
Author

Choose a reason for hiding this comment

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

After further investigation, I think this change + the types/react PropsWithRef definition does fix the original issue.

However, it also revealed a related problem: several MUI component props refer to React types that are not currently supported by compat (e.g. React.InputHTMLAttributes). The simple fix would be to alias those to HTMLAttributes, which captures all valid HTML attributes already (per #4202). A more React-y way would be to add React's stricter tag-based props there's quite a lot of them and it this seems like a big change in typing complexity over the current state.

@rschristian do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

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

Aliasing to the looser HTMLAttributes sounds fine for the time being, if you're willing.

Indeed, supporting all of those per-tag attributes would be a PITA, though (eventually) desirable I guess.


export function flushSync<R>(fn: () => R): R;
export function flushSync<A, R>(fn: (a: A) => R, a: A): R;
Expand Down