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

Type definition of "withApollo" HOC seems wrong #3392

Open
async3619 opened this issue Aug 19, 2019 · 5 comments · May be fixed by #3538
Open

Type definition of "withApollo" HOC seems wrong #3392

async3619 opened this issue Aug 19, 2019 · 5 comments · May be fixed by #3538

Comments

@async3619
Copy link

async3619 commented Aug 19, 2019

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.

@arieslx
Copy link

arieslx commented Aug 28, 2019

I got this error too, can u share your solution

the problem in my project is type error,so I try to

class MyMeeting xxx

export default withApollo(MyMeeting as any)

not elegantly but usefull

@async3619
Copy link
Author

async3619 commented Sep 3, 2019

@arieslx I've redeclared whole withApollo function definition like this:

import React from "react";
import { OperationOption, withApollo as withApolloImpl } from "react-apollo";

import { ApolloClient } from "apollo-client";

export declare type WithApolloClient<P> = P & {
    client: ApolloClient<any>;
};

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

@hwillson
Copy link
Member

If anyone wants to submit a PR that fixes this, we'll definitely review it - thanks!

@liweinan0423 liweinan0423 linked a pull request Sep 27, 2019 that will close this issue
@async3619
Copy link
Author

I don't want to put the blame on anybody but my issue opened almost 6 months ago. I'm really shameful about I don't know how to open pull request even if I'm using git like 2 years.

I don't think this is a big problem/issue (or bug) that break something that working with this module but I think we should care about type errors since this module written on typescript.

Funnily enough, I don't even know whether if this error still occurring with latest typescript version or not. 😂 and I respect all the people who contributed to the project, and I didn't write this comment aggressively either. I hope you guys don't misunderstand my purpose.

...so any updates on this issue?

@blackmagic0
Copy link

So I had been following this issue for a while, I managed to work around it by doing the following:

import React from "react";

interface MyProps {
    something: boolean;
}

const MyComponent = ( props : WithApolloClient<MyProps> ) => {
    const x = client.query( ... );
    return ...
}

export default withApollo<MyProps>( MyComponent );

I have since moved on from this ugly approach to just using the hook:

import React from "react";
import { useApolloClient } from "react-apollo";

interface MyProps {
    something: boolean;
}

const MyComponent = ( props : MyProps ) => {
    const client = useApolloClient();
    const x = client.query( ... );
    return ...
}

export default MyComponent;

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants