Return undefined for result.data if no data is available

[danilobuerger]

This is to align with react-apollo and the decision that data should be TData | undefined instead of TData | {}.

Ref:

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

Related:

• Fix type definition for QueryResult data property react-apollo#2734
• Add test that demonstrates why query result types are wrong react-apollo#2746

[hwillson]

Just re-capping the discussion we had in Slack about this - we're a bit worried about backwards compatibility with regards to adjusting the types of currentResult (since we don't want to major version bump for this). @danilobuerger suggested a workaround

getCurrentQueryResult defines data as any right now, se we can get away with it by actually specifying the type.

For currentResult we could add another function like getCurrentResult which returns the correct typings. Then adding a deprecation warning to currentResult and having it just call getCurrentResult changing undefined to {}. We can then remove it later at a semver bump.

and I like it! So 👍 on this approach @danilobuerger - thanks!

[danilobuerger]

@hwillson Implemented at discussed. This is now ready for review. After merge, I will take care of react-apollo side to use this new API.

[hwillson]

Thanks for this @danilobuerger - looks great!

[nuragic]

@danilobuerger @hwillson

So far, the docs specify the following:

data: TData
An object containing the result of your GraphQL query. Defaults to an empty object.

https://www.apollographql.com/docs/react/api/react-apollo.html#query-render-prop

However, this change seems to have shipped and then reverted due to a breaking change for TS users: apollographql/react-apollo#1983 (comment)

I can't believe there's no simple way to tell TS that an object can be "empty" by default, and undefined is preferred... wow.

I really don't get it... take in mind that for people like me AKA end users who are using JavaScript, it's definitely worse to have data defaults to undefined instead of {}.

Anyway, I guess the docs should be fixed?

Thanks!

[nuragic]

😕

https://github.com/apollographql/react-apollo/blob/3985b47f086f38f891099732842ca2f16c7966c4/src/Query.tsx#L89

  // 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?

[danilobuerger]

@nuragic

I can't believe there's no simple way to tell TS that an object can be "empty" by default, and undefined is preferred... wow.

There is a simple way to tell TypeScript (TData | {}). However, it is far harder to then discriminate between {} and TData. Instead of checking if (data) {...} TypeScript users would have to write a type guard to check if its TData.

I really don't get it... take in mind that for people like me AKA end users who are using JavaScript, it's definitely worse to have data defaults to undefined instead of {}.

The library should be useable by both JavaScript and TypeScript users. We fell that this is a good compromise for both worlds. If you have a better idea which works for both worlds, we would really like to hear about it.

Also, I don't really see the argument for JavaScript. If you do data.a.b it would still error out, as a would then be undefined. So you would still have to check it. In my opinion it is better to fail early.

Anyway, I guess the docs should be fixed?

Yes. Thanks for reporting it.

[nuragic]

Thanks for the quick reply! I'll try to elaborate more later (sorry, already unavailable right now) on why having {} instead of undefined would be a better default (it involves nested Query components using the skip option).

[danilobuerger]

@nuragic Sure, but please not here. This is a closed, merged PR. Not a place to discuss this. If you like, please open a feature request at https://github.com/apollographql/apollo-feature-requests