-
Notifications
You must be signed in to change notification settings - Fork 787
Fix type definition for QueryResult data property #2734
Conversation
@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/ |
We are not going to merge this. Please see #2424 and related for the reasoning. |
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! |
The type definition is not wrong though. It is exactly what it is supposed to be. We either get the whole
This is not correct anyway. You can't assign an empty object here. |
@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 |
I don't understand your question. If you get back an empty object instead of |
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.. |
Query result is an empty object or TData, not undefined or TData.
I ran into a problem with the current type definition.
Partial makes each of its generic type's properties optional
I could ofcourse change my TData type to have optional keys:
But then it doesn't directly reflect what response my graphql server gives, and in my case the types are generated..