improve QueryResult data typescript typing

[jzimmek]

The current QueryResult typing defines

react-apollo/src/Query.tsx

Lines 89 to 97 in a4c8703

	// we create an empty object to make checking for data
	// easier for consumers (i.e. instead of data && data.user
	// you can just check data.user) this also makes destructring
	// easier (i.e. { data: { user } })
	// however, this isn't realy possible with TypeScript that
	// I'm aware of. So intead we enforce checking for data
	// like so result.data!.user. This tells TS to use TData
	// XXX is there a better way to do this?
	data: TData | undefined;

data: TData | undefined;

I am not super deep into the react apollo codebase, but I this can eventually be solved by using "Discriminated Unions" in typescript: http://www.typescriptlang.org/docs/handbook/advanced-types.html

The "discriminant" attribute (eg. "resultType") would have two possible values: "data" | "error"

(Eventually even a third one: "data-and-error" if this is a valid state in apollo)

Anyone already investigated this as a possible solution?

[dallonf]

This is definitely the wrong type for this; it just bit me.

Given the server-side types:

type Query{
  mapSearch(name: String!): MappingSearchList!
}

type MappingSearchList {
  data: [MappingSearchEntity!]!
}

This typing causes the following line to pass, even though it contains a major type error (note: I'm using apollo-codegen to generate TS types for this component's query):

const items = (data && data.mapSearch.data) || [];

Note that mapSearch can be undefined because data can be {} when it comes from Apollo Client, even though mapSearch is non-nullable according to the schema (which imo is wrong for the schema, but that's not really the point here).

I think the line @jzimmek mentioned should be data: TData | {}. I'll do some experiments to see if that works and is convenient to use...

Update: Ah, I see the difficulty. TypeScript doesn't recognize TData | {} as a discriminated union.

Here's a possible solution that doesn't break the public API for JS users (unless you happen to be querying a field called loaded... hm):

// react-apollo typedefs
export type QueryResultData<TData> = ({ loaded: true } & TData) | { loaded: false };
export interface QueryResult<...> {
  // ...
  data: QueryResultData<TData>
  // ...
}

// in a component
const items = (data.loaded && data.mapSearch.data) || [];

[bbqbaron]

I too have been struggling with the very permissive types; I would love to see something union-based. For what it's worth, one thing that's stopped me has been the fact that, between skip, refetching, and all-based error handling, the type of a query result seems like it might genuinely not be a union-able type. Ie, error, loading, and data may all freely vary independently of each other. Even if that's true, though, I'd still like to make the types sufficiently paranoid and say "I can promise nothing; at the react-apollo edge, everything is possibly undefined" and require TS users to do typesafe discrimination downstream. Thoughts?

[danilobuerger]

We are going to stick to TData | undefined. See following for more details and discussion:

• Query: Fix data is undefined on error #1983
• Use Partial<TData> rather than TData | {} #2313
• Revert "Use Partial<TData> rather than TData | {} (#2313)" #2423

[eltonio450]

Hi @danilobuerger ,

I am not sure to fully understand why you want to stick to TData | undefined, it's kinda lost in the threads...

Furthermore, it seems wrong, no ? We have type error at runtime, because we check if data is defined, and then if ok, we use data.object_A.object_B

Are we the only one in this case ?

[danilobuerger]

Its not wrong. If you get anything other than TData | undefined please file a bug with a reproducible sample. Please bear in mind that there is an open PR in apollo-client changing the last pieces to TData | undefined (from TData | {}) as well.

[eltonio450]

ok we have {} with the last versions of apollo-client and react-apollo, but I am lost between all the PR's and issues, so I'll wait for your PR to be merged and check for {} in our wrapper til then

Thank you for your help !

[danilobuerger]

@eltonio450 you can follow apollographql/apollo-client#4356

[eltonio450]

yes, done, thank you :)