Revert "Use Partial<TData> rather than TData | {} (#2313)"

[rosskevin]

This reverts commit 2f15d9f.

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

#2313 introduced use of Partial for TData on a query result which makes all properties optional and is not correct or equal to the previous TData | {}.

@hwillson we should get a patch out asap as this is a breaking change in 2.2.0 for all users who are strongly typed.

/cc @tgriesser @donataswix

[hwillson]

Great - thanks @rosskevin!

[jackh726]

@hwillson @rosskevin
So, while TData | {} is not the same as Partial<TData>, both are breaking changes compared to 2.1.11 (with TData | undefined).

This is because before this could be done:
const { inner } = result.data!;
This would have inner be typed correctly, under the assumption it exists on data.

With 2.2.0, the above code would give inner a type of typeof inner | undefined. So any code that doesn't check for this fails. If all the properties of TData were | null anyways, this actually doesn't end up breaking much, but is still a breaking change nonetheless.

With 2.2.1, the above code isn't allowed at all because Property 'inner' does not exist on type '{}'. The only way to fix this is to explicitly narrow the type to TData by checking 'inner' in result.data. This breaks any code that doesn't explicitly narrow the scope.

Is it intended to have a breaking change in a minor release?

[hwillson]

@jackh726 No, breaking changes were not intended. We were a bit concerned about the TData | undefined to TData | {} change being breaking (see the comments in #1983), but in the end it seemed like the probability was low. Apparently not ☹️.

[rosskevin]

@jackh726 I didn't know the data was previously TData | {}, I just looked to recent PRs and knew for sure that Partial was not the answer.

What I ended up with to see if data was loaded was Object.keys().length then finally render my components and cast as TData in our internal wrapper. My code is too specific but it might be of use in this conversation as an example of what is being done outside react-apollo and how we might type it/make it better. Here's a gist:
https://gist.github.com/rosskevin/54e7ff04aa0a914d43ad213b5163b17a

Definitely no intended breakage, but it would be good to get some consensus on a way forward. @excitement-engineer may have some thoughts too.

[alamothe]

While GraphQL allows for partial result, I doubt many use this feature. Partial, although correct, would require everyone to handle partial results.

Related and interesting problem: previously we just checked if data was undefined and then avoid rendering the component. Now (after recent changes) we have to check for {} instead (not trivial, have to use lodash).

But it seems this is no longer enough either, as sometimes data will only have a Symbol('id') entry! 😮 So it gets harder and harder to check if we have data or not. I might even go as far as @rosskevin and check for Object.keys().length...

[alamothe]

It would be great if there was a simple boolean field saying if data is available or not (I don't think this is just problem with the typings).