Fields can be undefined but return type of getters can't

[renkei]

What is the idea behind the current implementation to generate code from proto files where the return type of the field getters is always cast to the data type of the field?

The cast in the generated typescript code is realized with an as expression, which can be dangerous. The Typescript compiler is possibly happy with the cast while the data type has nothing to do with the real data type available at runtime in JavaScript.

This is exactly the case when a field of a proto message, here B, is of type of another proto message, here A:

message A
{
    int32 id = 1;
}

message B
{
    A item = 1;
}

If I use the generated typescript code to create an instance of B like:

const b = new B();

then the generated typescript code tells me that b.item is always of type A but in fact, b.item is undefined. This can be checked easily by printing b.item to console. So, the correct typings for b.item would be A | undefined. Or am I wrong? Do I miss something here?

[renkei]

It's even worse. Let's say, I know that the Typescript Compiler can't help me here because of the wrong typings in the generated code but it's OK, because I will do the check manually in the code. Then ESLint tells me that I'm doing something wrong:

// Unnecessary conditional, the types have no overlap. (eslint@typescript-eslint/no-unnecessary-condition)
if (b.item === undefined) {
    ...
}

// Unnecessary conditional, value is always truthy. (eslint@typescript-eslint/no-unnecessary-condition)
if (b.item) {
    ...
}

[thesayyn]

I am not sure if that is the correct approach here as we tried that one before. The best would be optional getters which is not supported by typescript yet.

See #20, #33

also microsoft/TypeScript#14417

[renkei]

I don't think that this is an issue of TypeScript. The typings should just reflect the types that can occur at runtime, otherwise the typings at compile time don't make any sense.

What I can see:

1. Singular Scalar Types
   Setters should accept T, getters should return T because all variants of T have default values.
2. Singular Message Types
   Setters should accept T | undefined, getters should return T | undefined because message types don't have default values. If a message type field is not set then the field returns undefined.
3. Repeated Fields
   Setters should accept T[], getters should return T[] because repeated fields always have a default value: an empty array via []. It doesn't matter if T is a scalar or message type.
4. Map fields
   Setters should accept Map<K, V>, getters should return Map<K, V> because map fields always have a default value: an empty map via new Map<K, V>().
5. oneof fields
   Setters for each possible field should accept T | undefined, undefined should unset the field, T should unset all other fields, getters should return T | undefined even for singular scalar types because this reflects an unset oneof field. In addtion, there should be a virtual field (not part of the proto message) that allows the user to write a switch/case to detect easily which oneof field is really set. Then the user can safely cast away the undefined from the type with as T. Or, as alternative, the user checks each oneof field for being not undefined. The user can stop the field check at the first field which is not undefined because it's the only one, that's the idea of oneof
6. proto3 optional fields
   Is just a shortcut for a oneof that contains only one field. Setters should accept T | undefined and getters should return T | undefined.
7. Well-Known Types, e.g. Int32Value
   For unset singular scalar types you (as a receiver / consumer of a message) cannot detect if the sender / producer of the mesage has set a default value or left the field unset. With e.g. Int32Value instead of int32 this can be detected, so setters should accept T (no support for unset, the user can just set a value or ignore the field) and getters should return T | undefined

I would say that all cases above are properly covered by the current feature set of TypeScript. What do you think?

[thesayyn]

I have no problem whatsoever with the approach above. there are some typescript limitations that prevent us from doing this.

The problem with getter setter types is that they can’t have different types. hence, since T | undefined is different than T type-checker will yell at you.

[thesayyn]

https://www.typescriptlang.org/play?#code/MYGwhgzhAECyCmUwHN7QN4ChrQvALtPovgBQBuYIArvAFy74BOAlgHbLQA+01bAJvABm7ePwCUGHAF9s0VIWIQy4hstYcMs6UA

Also see: microsoft/TypeScript#2521

microsoft/TypeScript#42425

[thesayyn]

it is going to take a while this gets stabilized. then we can revisit the options.

[renkei]

Ok, that's true, the type of getters and setters must be equal. If I look at my check list above then only point 7 (Well-Known Types) is affected by this limitation. So, the setter must also accept undefined. That's not really an issue as long as the getter returns the correct typings so that an user of your package can safely work with returned values.

The main reason for this ticket is point 2 (Singluar Message Types) because this is really an issue. Here, the typings do not reflect the returned types at runtime. So, a customer of your package has to find a workaround.

To make it short: Accept undefined for setters of Well-Known Types, then you have the situation that all 7 items from the check list above have the same types for getters and setters. So everything can solved with current TypeScript versions. And this ticket could be closed once you switch from T to T | undefined for getters and setters of Scalar Message Types. Sorry, I still don't see the need to wait for new TypeScript features. Maybe I'm wrong.

[thesayyn]

Sorry, I still don't see the need to wait for new TypeScript features. Maybe I'm wrong.

You are right. This definitely should be fixed. Would be happy to see a PR if you would like to take it. Otherwise, I will try to take this, this week.

[thesayyn]

Though this is going to be a breaking change. so I have to find a way to land this without annoying everyone.

[thesayyn]

7. Well-Known Types, e.g. Int32Value
   For unset singular scalar types you (as a receiver / consumer of a message) cannot detect if the sender / producer of the mesage has set a default value or left the field unset. With e.g. Int32Value instead of int32 this can be detected, so setters should accept T (no support for unset, the user can just set a value or ignore the field) and getters should return T | undefined

with proto3 optionals fields also get a getter called has_[name]