typescript-generator icon indicating copy to clipboard operation
typescript-generator copied to clipboard

Unexpected nullabilities of maps

Open danielkaneider opened this issue 4 years ago • 3 comments

Hi, I've created a new test taking NullabilityTest.test as an example. Instead of A, I've created the following two Java classes

    private enum EnumC {
        A, B, C;
    }

    private static class C {

        public Map<String, String> map1;
        public Map<EnumC, String> map2;
    }

This results in the following output:

interface C {
    map1: { [index: string]: string };
    map2: { [P in EnumC]?: string };
}

type EnumC = "A" | "B" | "C";

I'd expect that map1 and map2 would be the same in terms of nullability. (What I'm actually trying to achieve is, that all Java maps should get nullable values by default)

danielkaneider avatar Feb 25 '20 15:02 danielkaneider

Hi,

Map with enum keys like { [P in EnumC]?: string } has optional properties because it contains mapping for any subset of the keys. Type of values is string | undefined because some keys may not be mapped. Another variant would be { [P in EnumC]: string } (without ?) but it would mean map containing all keys and would not allow for example following assignment: c.map2 = { A: "A" }. So I consider this case correct.

Map with string keys like { [index: string]: string } is "standard" way how to describe object with any number of string keys mapped to string values. Type of values is just string which is not exactly precise because value can be undefined when key is not present but that's how TypeScript works. Another variant would be { [index: string]?: string } (with ?) but it is not valid TypeScript code.

Until now it had nothing to do with nullability feature of typescript-generator. With nullability feature it is possible to express nullability of map values. Using this Java code:

class C {
    public Map<String, @Nullable String> map1;
    public Map<EnumC, @Nullable String> map2;
}

it is possible to generate for example following TypeScript code (depending on nullabilityDefinition parameter):

interface C {
    map1: { [index: string]: string | null };
    map2: { [P in EnumC]?: string | null };
}

Please note that @Nullable annotation needs to have TYPE_USE target.

By default types without nullable annotation are non-nullable. This is consistent with Kotlin and TypeScript. In Kotlin String is non-nullable and String? is nullable. In TypeScript strict mode string is non-nullable and for example string | null allows null value.

I am not sure if this answer solves your case but I at least hope it clarifies reasons and describes how maps and nullability work in typescript-generator.

vojtechhabarta avatar Feb 25 '20 19:02 vojtechhabarta

Thank you for the very detailed explanation. Just to point it out, I'm perfectly fine with the current conversion of Enum maps.

Given the following examples of data transferred for a map with a and b:

{"a": 1, "b": 2}
{"a": 1, "b": null}
{"a": 1}

Java

  1. The first case could be represented by Java Map<String, String> which covers the two cases for a and b, but does not handle the case of c.
  2. The second case would be something like Java Map<String, @Nullable String>. Again, there is no workaround for c, as in TS undefined and null are treated differently.
  3. The third case is similar to other cases, and could be represented by Map<String, String>

Typescript

  1. This could be { [index: string]: string }. In order to cover c this could result in { [index: string]: string | undefined}
  2. This would be { [index: string]: string | null }. In order to cover c this could result in { [index: string]: string | undefined | null}
  3. The third case would be the same as (1), i.e. { [index: string]: string | undefined}

Final words

I do see a valid use-case for @Nullable annotation. IMHO it just covers very few use-cases. Moreover, I think it could be dangerous to mix null and undefined (by misusing NullabilityDefinition). Possible solutions could be:

  • make all Maps with UnionType | undefined by default. Maybe this default could be specified by some setting. A possibility could be to combine this new default with a specific override using nonNull from #443
  • create undefinedAnnotations, along nullableAnnotations which does the similar thing

danielkaneider avatar Feb 26 '20 10:02 danielkaneider

I think basically there are 3 places where we deal with null and undefined values:

  • properties
  • lists
  • maps

I would skip properties for now. For lists and maps single setting might not be optimal (parameter nullabilityDefinition). Any variant that includes undefined value doesn't make sense for lists since JSON array cannot contain undefined.

[
    "a",
    null,
    undefined // invalid
]

JSON objects are similar:

{
    "a": "a",
    "b": null,
    "c": undefined // invalid
}

But for maps undefined can be useful because it is returned when accessing key which is not present in the map. On the other hand when iterating over the object we can be sure that undefined value will not be returned. That's also how TypeScript deals with it.

interface A {
    map: { [index: string]: string | null };
}
declare const a: A;
const x = a.map["x"];
for (const key in a.map) {
    const value = a.map[key];
}

Here type of x is string | null (not string | null | undefined) even though x can be undefined. Also type of value is string | null but here it cannot contain undefined value.

So I think using just null for nullability when representing JSON data might be a good solution because it represents well map iteration scenario (but not map random access).

Another good compromise could be to use null and undefined even though it doesn't make sense for lists but it represents well random map access and it might be useful for properties.

Since this feature is still relatively new in typescript-generator I would not do any enhancements (meaning adding new parameters) until we have more feedback.

vojtechhabarta avatar Feb 26 '20 13:02 vojtechhabarta