Fix type definition for QueryResult data property

[LarsJK]

Query result is an empty object or TData, not undefined or TData.

I ran into a problem with the current type definition.

interface MyResult {
  item: {
    name: string;
  };
}

<Query<MyResult, ..>>
{({ data }) => {
  // The old type of data would be MyResult | undefined so I unwrap it like this:
  const name = data && data.item.name || 'default name'
  // TypeError: Cannot read property 'name' of undefined
}}
</Query>

Partial makes each of its generic type's properties optional

I could ofcourse change my TData type to have optional keys:

interface MyResult {
  item?: {
    name: string;
  };
}

But then it doesn't directly reflect what response my graphql server gives, and in my case the types are generated..

[apollo-cla]

@LarsJK: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[danilobuerger]

We are not going to merge this. Please see #2424 and related for the reasoning.

[LarsJK]

Could you please reconsider this @danilobuerger?

Having a type definition that is wrong and can not be trusted is dangerous. As you can see in my example it causes runtime crashes, which are exactly the kind of things typescript helps to prevent.

The referenced issue is also not the same as this implementation so it doesn't really give any reason to why this should be closed...

Given an interface:

interface Foo {
  bar: {
    baz: number;
  };
}

var foo: Foo | {} = { bar: { baz: 100 }}
foo.bar.baz
// error TS2339: Property 'bar' does not exist on type '{} | Foo'.
// Property 'bar' does not exist on type '{}'.

var foo: Foo | undefined = {}
foo.bar.baz
// With strict null checks TS warns me about foo: Object is possibly 'undefined'.

// So I try too first check for undefined:
foo && bar.baz
// Now TS is happy but this is actually worse 😱:
// I get runtime crashes: TypeError: Cannot read property 'baz' of undefined

var foo: Partial<Foo> = {}
foo.bar.baz
// With strict null checks TS warns me about bar instead of foo: Object is possibly 'undefined'.

// So I try too check bar for undefined this time:
foo.bar && foo.bar.baz
// Now TS is happy and..
// Yay no runtime crashes 💯 

So a partial directly reflects the value that is actually used, it doesn't loose type and it can prevent runtime crashes!

[danilobuerger]

Having a type definition that is wrong and can not be trusted is dangerous.

The type definition is not wrong though. It is exactly what it is supposed to be. We either get the whole TData or undefined.

var foo: Foo | undefined = {}

This is not correct anyway. You can't assign an empty object here.

[LarsJK]

@danilobuerger do you ever do that tho? I always get back an empty object, and when I get an empty object it is wrong.. it's not TData and it's not undefined.. If you do assign an empty object then it should be Partial<TData> | undefined..

[danilobuerger]

I don't understand your question. If you get back an empty object instead of TData or undefined thats a bug and doesn't mean the typings are wrong. If thats the case open a bug report with a reproducible sample.

[LarsJK]

Okay thanks. Will open a bug then. The way I understood it the empty object return was added as a feature to allow safe destruction of data.

That is also what the code comment said..