DefinitelyTyped
[react] Fix contextually inferred types of forwardRef arguments
Please fill in this template.
- [x] Use a meaningful title for the pull request. Include the name of the package modified.
- [x] Test the change in your own code. (Compile and run.)
- [x] Add or edit tests to reflect the change.
- [x] Follow the advice from the readme.
- [x] Avoid common mistakes.
- [x] Run
pnpm test <package to test>.
TypeScript 5.5 introduced support for isolated declarations. Code that benefits from isolated declarations must explicitly annotate exported variables when the types can't be trivially inferred from syntax. This means that there will be an increased need for React components to be given explicit type annotations and greater usage of the utility types that @types/react provides.
One of the shortcomings I've come across is when using forwardRef, the inferred type of props incorrectly includes ref. This PR contains a fix for that.
Unfortunately, the representation of the type is suboptimal because the typing of ForwardRefExoticComponent doesn't separately take the prop type and ref type and requires you to pre-combine them as something like ForwardRefExoticComponent<Props & React.RefAttributes<RefType>>. In TypeScript, it's not generally possible to extract a type in an intersection back out of the intersection, which means that we can't pull Props out of Props & React.RefAttributes<RefType> with something like extends infer TProps & React.RefAttributes<infer TRefType>. Going forward, I would recommend providing a new utility type, ForwardRefComponent<Props, RefType> which preserves Props. That would also be an opportunity to deprecate ForwardRefExoticComponent whose name I've always found to be needlessly verbose and intimidating.
@MichaelMitchell-at Thank you for submitting this PR!
This is a live comment that I will keep updated.
1 package in this PR
Code Reviews
Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.
You can test the changes of this PR in the Playground.
Status
- ✅ No merge conflicts
- ✅ Continuous integration tests have passed
- ✅ Most recent commit is approved by a DT maintainer
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.
Diagnostic Information: What the bot saw about this PR
{
"type": "info",
"now": "-",
"pr_number": 70361,
"author": "MichaelMitchell-at",
"headCommitOid": "7437ad1e6ac24f96b6a1d8627cf1dcba48980707",
"mergeBaseOid": "8b46b339d7432e545c92cdfee8a461bc3bd6037c",
"lastPushDate": "2024-08-23T05:59:07.000Z",
"lastActivityDate": "2024-08-30T09:45:04.000Z",
"mergeOfferDate": "2024-08-30T00:20:04.000Z",
"mergeRequestDate": "2024-08-30T09:45:04.000Z",
"mergeRequestUser": "MichaelMitchell-at",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "react",
"kind": "edit",
"files": [
{
"path": "types/react/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/test/index.ts",
"kind": "test"
},
{
"path": "types/react/ts5.0/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/ts5.0/test/index.ts",
"kind": "test"
}
],
"owners": [
"johnnyreilly",
"bbenezech",
"pzavolinsky",
"ericanderson",
"DovydasNavickas",
"theruther4d",
"guilhermehubner",
"ferdaber",
"jrakotoharisoa",
"pascaloliv",
"hotell",
"franklixuefei",
"Jessidhia",
"saranshkataria",
"lukyth",
"eps1lon",
"zieka",
"dancerphil",
"dimitropoulos",
"disjukr",
"vhfmag",
"hellatan",
"priyanshurav",
"Semigradsky",
"mattpocock"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "gabritto",
"date": "2024-08-30T00:19:28.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "eps1lon",
"date": "2024-08-29T12:26:50.000Z",
"abbrOid": "9748f7a"
}
],
"mainBotCommentID": 2306349414,
"ciResult": "pass"
}
🔔 @johnnyreilly @bbenezech @pzavolinsky @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @eps1lon @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan @priyanshurav @Semigradsky @mattpocock — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.
@MichaelMitchell-at One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!
@eps1lon Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?
👋 Hey folks, didn't want to post a full issue right off the bat, but wanted to flag that this seems to have broken our current approach to polymorphic forwardRef typings -- would appreciate any thoughts on getting around the new error: TS playground
👋 Hey folks, didn't want to post a full issue right off the bat, but wanted to flag that this seems to have broken our current approach to polymorphic forwardRef typings -- would appreciate any thoughts on getting around the new error: TS playground
One workaround I found is to pass type parameters to React.forwardRef, e.g. export const Component: Component = React.forwardRef<HTMLElement>(
~~Hmm, I can't get that to behave in the playground I linked?~~
Edit: ah, it wants HTMLAnchorElement specifically to match the default for as. Went with export type ReactForwardedElement<C extends React.ElementType> = C extends keyof HTMLElementTagNameMap ? HTMLElementTagNameMap[C] : C; so I have a decent generic for it, and then React.forwardRef<Polymorphic.ReactForwardedElement<ComponentDefault>>. Thanks!
Hmm, I can't get that to behave in the playground I linked?
Oh and also remove the default type parameter on the next line,
export const Component: Component = React.forwardRef<HTMLElement>(
<C extends React.ElementType>(
For more correctness you can also change ComponentProps<C> to Polymorphic.PolymorphicProps<C, CoreComponentProps> on line 43
Proposed solution doesn't work for non-optional props. Just try to remove optional quantifier ? from hello prop: Playground
It broke our main withForm() HOCs. After adding PropsWithoutRef<P> wrapper, props cannot be inferred at all.
Please checkout this Playground
Do you folks know how to fix it?
It broke our main
withForm()HOCs. After addingPropsWithoutRef<P>wrapper, props cannot be inferred at all.Please checkout this Playground
Do you folks know how to fix it?
Hi @MichaelMitchell-at, very sorry to ping you here directly - but as far as I can tell, this change has broken polymorphic types. I tried implementing your suggested changes above and didn't have much luck. Please let me know if you see a solution here, or a mistake I've made. For now we've pinned our React types to 18.3.4
Hi @MichaelMitchell-at, very sorry to ping you here directly - but as far as I can tell, this change has broken polymorphic types. I tried implementing your suggested changes above and didn't have much luck. Please let me know if you see a solution here, or a mistake I've made. For now we've pinned our React types to 18.3.4
Sorry, I'm unable to see any difference in behavior with the playground you've linked, before and after uncommenting the designated lines.
Sorry, I'm unable to see any difference in behavior with the playground you've linked, before and after uncommenting the designated lines.
@MichaelMitchell-at Thanks for your response. The playground linked above is implementing the solution you recommended, and doesn't seem to work for non-optional props. I guess my question is, after these changes to the React types, how is it possible to achieve polymorphic components that work with both optional and non-optional props?
Uncommenting the old typings aren't enough to fix the polymorphic types in the playground - the technique we used to achieve polymorphic components is a bit different in 18.3.4. I can create another playground demonstrating that technique.
Okay here is a clearer ts playground demonstrating polymorphism working with the old types / patterns.
If you comment out the old types, you'll see the issue.
After commenting out the old types, you can try passing the element type to Box's forwardRef:
export const Box: BoxComponent = React.forwardRef<ReactForwardedElement<BoxDefaultElement>>(
I haven't found a way to resolve these issues and keep the strong polymorphic typings
Okay here is a clearer ts playground demonstrating polymorphism working with the old types / patterns.
If you comment out the old types, you'll see the issue.
After commenting out the old types, you can try passing the element type to
Box'sforwardRef:export const Box: BoxComponent = React.forwardRef<ReactForwardedElement<BoxDefaultElement>>(I haven't found a way to resolve these issues and keep the strong polymorphic typings
Since you're not going to get strong enforcement of the generic render function anyway, I would say your goal should merely be to avoid the type errors at the definition. You can do so by providing more general types explicitly to forwardRef. Playground
Since you're not going to get strong enforcement of the generic render function anyway, I would say your goal should merely be to avoid the type errors at the definition. You can do so by providing more general types explicitly to forwardRef. Playground
Thank you so much for your help and patience!
@MichaelMitchell-at , this is breaking our withTracking utility. We use withTracking to easily integrate react-tracking with our component library.
Argument of type 'PropsWithoutRef<TProps & TrackingProps>' is not assignable to parameter of type 'TProps'.
'PropsWithoutRef<TProps & TrackingProps>' is assignable to the constraint of type 'TProps', but 'TProps' could be instantiated with a different subtype of constraint '{ [key: string]: any; }'.
Type 'Omit<{ [key: string]: any; } & TrackingProps, "ref">' is not assignable to type 'TProps'.
'Omit<{ [key: string]: any; } & TrackingProps, "ref">' is assignable to the constraint of type 'TProps', but 'TProps' could be instantiated with a different subtype of constraint '{ [key: string]: any; }'.
Type '({ ref: ForwardedRef<any>; testID: any; } & PropsWithoutRef<TProps & TrackingProps> & { [P in keyof HandlersToAction<Trackables, TProps>]: (event: any) => void; }) | ({ ...; } & ... 1 more ... & { ...; })' is not assignable to type 'IntrinsicAttributes & TProps'.
Type '{ ref: ForwardedRef<any>; testID: any; } & PropsWithoutRef<TProps & TrackingProps> & { [P in keyof HandlersToAction<Trackables, TProps>]: (event: any) => void; }' is not assignable to type 'IntrinsicAttributes & TProps'.
Type '{ ref: ForwardedRef<any>; testID: any; } & PropsWithoutRef<TProps & TrackingProps> & { [P in keyof HandlersToAction<Trackables, TProps>]: (event: any) => void; }' is not assignable to type 'TProps'.
'{ ref: ForwardedRef<any>; testID: any; } & PropsWithoutRef<TProps & TrackingProps> & { [P in keyof HandlersToAction<Trackables, TProps>]: (event: any) => void; }' is assignable to the constraint of type 'TProps', but 'TProps' could be instantiated with a different subtype of constraint '{ [key: string]: any; }'.
The only thing I've found that can fix this is simply doing as TProps but that feels dirty.
@trcoffman I think you can change the signature to:
export function withTracking<
Trackables extends TrackablesBase,
TProps extends { [key: string]: any } & TrackingProps & {ref?: React.Ref<any>},
>(Component: ComponentType<React.PropsWithoutRef<TProps> & {ref?: TProps['ref']}>, config: WithTrackingConfig<Trackables, PropsWithoutRef<TProps>> = {}) {