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

Higher order type inference from generic React component #30650

Closed
OliverJAsh opened this issue Mar 29, 2019 · 9 comments · Fixed by #31116
Closed

Higher order type inference from generic React component #30650

OliverJAsh opened this issue Mar 29, 2019 · 9 comments · Fixed by #31116
Assignees
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Mar 29, 2019

TypeScript Version: 3.4.1

Search Terms:

Code

interface Component<P> {
    (props: P): unknown;
    // Comment this out and it works
    defaultProps?: Partial<P>;
}
declare function myHoc<Props>(C: Component<Props>): Component<Props>;

type Props<T> = { t: T; foo: number };
declare function MyComponent<T>(props: Props<T>): unknown;
// Expected type: <T>(props: Props<T>) => unknown
// Actual type: Component<Props<{}>>
const MyComponent2 = myHoc(MyComponent);

// Workaround: assert the resulting type
const MyComponent3 = myHoc(MyComponent) as typeof MyComponent;
@ahejlsberg
Copy link
Member

This is working as intended. We only make higher order function type inferences when the source and target types are both pure function types, i.e. types with a single call signature and no other members. We could potentially consider expanding our definition of pure function types to include optional properties. I'm going to mark this as a suggestion.

@OliverJAsh
Copy link
Contributor Author

Thanks @ahejlsberg. I also discovered the same problem when using generic React class components. Is that part of this issue or should it be filed separately?

declare class Component<P> {
    constructor(props: P);
}

interface ComponentClass<P> {
    new (props: P): Component<P>;
}

declare function myHoc<Props>(C: ComponentClass<Props>): ComponentClass<Props>;

{
    type Props<T> = { t: T; foo: number };
    class MyComponent<T> extends Component<Props<T>> {}

    // Props generic is lost! :-(
    // ComponentClass<Props<{}>>
    const MyComponent2 = myHoc(MyComponent);
}

@ahejlsberg
Copy link
Member

So, higher order function type inference is the ability to infer type arguments for the composing function that reference new inferred type parameters. Those new type parameters are then promoted onto the (single) signature of the output type of the composing function. This means we need to ensure that the type parameters are only referenced in that signature. One way to ensure this is to not permit other members in the output type, and that's what we currently do. We could potentially allow other members to be present, but we wouldn't be able to make higher order type inferences to any type parameters that are referenced by those properties. So, it wouldn't help your original example:

interface Component<P> {
  (props: P): unknown;
  defaultProps?: Partial<P>;
}

Here, we wouldn't be able to make higher order inferences for P because P is referenced outside the call signature (where we'd be attaching the new inferred type parameters). In a sense this reveals why the defaultProps pattern doesn't work for higher order components--you can't supply a default for a type you don't yet know.

Regarding the other question about higher order type inferences for constructor functions, that's an orthogonal issue, but definitely something I think we should support. But it would be subject to the same restrictions, i.e. output type can only have a single signature and no other members. We don't need a separate issue, we can just track it here.

@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Apr 25, 2019
@ahejlsberg ahejlsberg added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Apr 27, 2019
@ahejlsberg
Copy link
Member

Now implemented in #31116, with the limitations mentioned in my comment above.

@OliverJAsh
Copy link
Contributor Author

@ahejlsberg IIUC, #31116 fixes #30650 (comment) but not #30650 (comment). Is that correct, or do I misunderstand?

If it is correct, should we keep this issue open, to track #30650 (comment)?

@ahejlsberg
Copy link
Member

It fixes what we can possibly fix in this issue, so no reason to keep it open. As I explain in #30650 (comment) we can't permit higher order inferences to type parameter that are used outside the signature we're inferring to (such as the use of P in defaultProps?: Partial<P>).

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented May 29, 2019

Sadly we won't be able to benefit from this change in React due to #30650 (comment).

(React component types have extra properties which reference the generic, like defaultProps.)

Someone else has opened an issue for this, so I guess we can continue to track this over there: #31468

@DanielRosenwasser Note we might want to remove the following excerpt from the 3.5 release notes, seeing as React can't currently benefit:

certain UI libraries like React can more correctly operate on generic class components

@edaqa-uncountable
Copy link

One of this issue or #31468 should be open. This issue still needs to be tracked as it's a highly upvoted concern in React DefinitelyTyped/DefinitelyTyped#37087

@BreakUps
Copy link

BreakUps commented Apr 1, 2022

Here, we wouldn't be able to make higher order inferences for P because P is referenced outside the call signature (where we'd be attaching the new inferred type parameters)

@ahejlsberg Sorry I don't undestand the logic of the reason? Could you please explain more detail?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants