Add missing PropsWithRef type to fix #4293 #4333
Conversation
| @@ -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>>; | |||
There was a problem hiding this comment.
Bear with me as I'm doing this from my phone, but I don't see PropsWithRef defined anywhere?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
#4271 introduces a bug where some types are incorrectly resolved.
Compared to the original types/react implementation, #4271's is missing an enclosing PropsWithRef in the return statement. The problems appear to resolve after restoring that line.
Fixes #4293