Skip to content
This repository was archived by the owner on Apr 13, 2023. It is now read-only.
This repository was archived by the owner on Apr 13, 2023. It is now read-only.

Type definition of "withApollo" HOC seems wrong #3392

Open
@async3619

Description

@async3619

Intended outcome:

First of all, I'd like to apologize my poor english skill. I'll have some time for fixing this poor-english-skill issue first. 😬

When we use withApollo HOC, it should return a new component type without client property at props type like below:

import { withApollo } from "react-apollo";

type Props = WithApolloClient<{}>;

class Foo extends React.Component<Props> {
    public render() {
        return null;
    }
}

const Wrapped = withApollo(Foo); // <-- this component shouldn't need `client` prop now.

const test = <Wrapped />; // <-- this shows no error at all as how I expected.

Actual outcome:

But above code snippet with latest apollo release won't exclude client prop in original props type.

import { withApollo } from "react-apollo";

type Props = WithApolloClient<{}>;

class Foo extends React.Component<Props> {
    public render() {
        return null;
    }
}

const Wrapped = withApollo(Foo);
//    ^^^^^^^
// type of `Wrapped` is now `React.ComponentType<Pick<unknown, never>, any>`.

const test = <Wrapped />; 
//    ^^^^
// this code won't make errors but it treated like a component with `any` as prop type.

I thought it's just an error just because there's no properties at generic argument of WithApolloClient, but It made an error at withApollo(Foo).

I checked type definition of withApollo HOC and I found a problem:

function withApollo<TProps, TResult = any>(
    WrappedComponent: React.ComponentType<WithApolloClient<Omit<TProps, "client">>>,
//                                         ^^^^^^^^^^^^^^^^ (1)
    operationOptions?: OperationOption<TProps, TResult>,
): React.ComponentClass<Omit<TProps, 'client'>>;
//                                   ^^^^^^^^ (2)

Firstly, see (1). We're already expecting that generic TProps should have client property. so I think we shouldn't use WithApolloClient as prop type of WrappedComponent parameter since TProps would have it.

I don't get why we omit client property of TProps even if we'll add it again with WithApolloClient. Are there any reasons?

and see (2). we don't know whether if generic TProps would have client property or not since TProps isn't extending anything. It can accept literally any types and I think that's why prop type of returned component will have unknown.

so I think the approach of this issue would be like this:

function withApollo<TProps extends WithApolloClient<any>, TResult = any>(
    WrappedComponent: React.ComponentType<TProps>,
    operationOptions?: OperationOption<TProps, TResult>,
): React.ComponentClass<Omit<TProps, "client">>;

with above definition, we enforce TProps generic having client property as required one. and just remove it from returned component type. I'm not sure above approach is perfect or not but I believe current one is wrong.

How to reproduce the issue:

Version

  System:
    OS: Windows 10
  Binaries:
    Node: 10.15.1 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.17.3 - C:\Users\User\AppData\Roaming\npm\yarn.CMD
    npm: 6.9.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: 42.17134.1.0
  npmPackages:
    apollo-cache: ^1.3.2 => 1.3.2
    apollo-cache-inmemory: ^1.6.2 => 1.6.2
    apollo-client: ^2.6.4 => 2.6.4
    apollo-link: ^1.2.12 => 1.2.12
    apollo-link-error: ^1.1.11 => 1.1.11
    apollo-link-http: ^1.5.15 => 1.5.15
    apollo-link-logger: ^1.2.3 => 1.2.3
    apollo-link-schema: ^1.2.3 => 1.2.3
    apollo-server: ^2.6.3 => 2.7.2
    apollo-server-express: ^2.6.3 => 2.7.2
    apollo-utilities: ^1.3.2 => 1.3.2
    react-apollo: ^3.0.1 => 3.0.1

FYI, I'm using typescript 3.5.3 now.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions