Typescript: ChildProps.data should not be optional

[choffmeister]

Intended outcome:

The typescript definition of ChildProps.data should be non-optional, since according to the documentation (http://dev.apollodata.com/react/api-queries.html#graphql-query-data) the data property has a loading/error subproperty to check for completion/errors.

Actual outcome:

The actual type definition is

export declare type ChildProps<P, R> = P & {
    data?: QueryProps & R;
    mutate?: MutationFunc<R>;
};

forcing users to always check if data is defined before being able to access data.loading or other properties.

One example from the documentation

render() {
  const { data: { loading, error, todos } } = this.props;
  if (loading) {
    return <p>Loading...</p>;
  } else if (error) {
    return <p>Error!</p>;
  } else {
    return (
      <ul>
        {todos.map(({ id, text }) => (
          <li key={id}>{text}</li>
        ))}
      </ul>
    );
  }
}

would fail if using typescript because of data?.

Version

• [email protected]
• [email protected]

[leoasis]

I'd like to add to this one that instead of data being optional, it is the results that should be optional, in the OptionProps case, TResult should have all nullable fields (for the case where they are loading).

export interface OptionProps<TProps, TResult> {
    ownProps: TProps;
    data: QueryProps & Partial<TResult>;
    mutate?: MutationFunc<TResult>;
}

Are you willing to accept a PR for this? Is there any caveat with this proposed change?

[choffmeister]

What you call Nullable<T> should be TypeScript's Partial<T> I guess.

[leoasis]

Yeah, duh, good call! haha will update

[choffmeister]

Did look up here https://www.typescriptlang.org/docs/handbook/advanced-types.html. Actually both Nullable and Partial exist (did only know about Partial). Still if I recall correctly the unloaded props are undefined, so Partial should be the correct one. If there are now objections I would create a PR tomorrow for that.

[Jontem]

Looks as this was merged and included in version 1.4.16 but data is still optional but now with Partial<R> ?

export type ChildProps<P, R> = P & {
  data?: QueryProps & Partial<R>;
  mutate?: MutationFunc<R>;
};

https://github.com/apollographql/react-apollo/blame/3e245205097d427daaac7cb89c0f97eea78032ae/src/types.ts#L70

Edit: Saw #1143 (comment) now. Looks like this is the reason

[leoasis]

Yeah, I think the reason it was set back to optional is that since the HOC is used for both queries and mutations, and since currently there's no way to tell Typescript how the HOC will act like, the type definitions need to allow both to be optional, since for queries you'll get data, and for mutations you'll get mutate.

[stale]

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

[jurosh]

@leoasis Maybe would be good to create another type for props with queries only Eg. ChildQueryProps<ResponseType> where types would be stronger, but not optional. Otherwise it's really complicated to work with those types in components.

Also this official example doesn't work now because of optional typings https://www.apollographql.com/docs/react/features/static-typing.html#classes-vs-functions

[leoasis]

@jurosh yeah the problem with that is that by design the graphql HoC works for both queries and mutations, and the decision to act as one of those is determined dynamically by inspecting the graphql document being passed in. If it has a mutation operation, then it acts as a mutation container, otherwise as a query container. This is not possible for Typescript to know in advance, which leads to these types that have everything optional. I think maybe the types could be improved a bit more so that at least the developer can specify if the types for the container should be for querying or for mutating, with a type parameter for example.

[stale]

This issue has been automatically labled because it has not had recent activity. If you have not received a response from anyone, please mention the repository maintainer (most likely @jbaxleyiii). It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

[rosskevin]

#1369 offers a refined test case that should be considered, though it is a duplicate of this issue. Actually, perhaps since it is succinctly stated perhaps we should close this one and keep that one open?

[rosskevin]

I'm closing this in favor of #1369 - it has a succinct test case. 2.1 roadmap looks like we'll have discrete components so this won't be a problem. Please follow #1369.