Prohibit `undefined` in TypeScript Index Signatures for `Data.Map` when key is `String`

[benrbray]

Thanks for aeson-typescript, it's great!

Example

I've noticed that when using Data.Map as follows:

newtype Example = Example (Map String Int)
$(deriveTypeScript defaultOptions ''Foo)
result = tt @Example

(helper functions)

tsFormatOpts :: FormattingOptions
tsFormatOpts = defaultFormattingOptions {exportMode = ExportEach, typeAlternativesFormat = EnumWithType}
ts :: (TypeScript a) => Proxy a -> String
ts = formatTSDeclarations' tsFormatOpts . getTypeScriptDeclarations
tt :: forall a. (TypeScript a) => String
tt = ts (Proxy @a)

The generated types look like this:

export type Example = IExample;
// actual generated type
export type IExample = {[ k in string] ?: number};

It's a bit difficult to use these types, since indexing IExample will give a number | undefined value, even though the ToJSON instance for Data.Map should never include undefined values.

const foo: Example = { bar : undefined } // no error

function example(data: Example) {
  const bar: { [k : string] : number } = data;
  // Type 'IExample' is not assignable to type '{ [k: string]: number; }'.
  // 'string' index signatures are incompatible.
  //   Type 'number | undefined' is not assignable to type 'number'.
  //     Type 'undefined' is not assignable to type 'number'.
}

Note that these errors still happen with exactOptionalPropertyTypes enabled.

Desired Behavior

Instead, I would have expected the generated type to look more like this:

// desired type
export type IExample = { [k : string] : number };

const foo: Example = { bar : undefined }
// Type 'undefined' is not assignable to type 'number'.

function example(data: Example) {
  const bar: { [k : string] : number } = data;
  // Type 'IExample' is not assignable to type '{ [k: string]: number; }'.
  // 'string' index signatures are incompatible.
  //   Type 'number | undefined' is not assignable to type 'number'.
  //     Type 'undefined' is not assignable to type 'number'.
}

Questions

• was there a particular reason for this choice of types?
• would it be possible to change the behavior when the key is String or an alias for String?

Related

microsoft/TypeScript#46969

[thomasjm]

Hmm, the question mark was introduced in this commit: 64c03f3

It's been a while so I don't remember the context well. It makes sense that : was changed to in but I'm not sure why the question mark was needed. I can't think of any situation where such a map would contain undefined.

I tried removing it locally and the tests passed ¯\(ツ)/¯. Trying it in CI here.

Before merging such a change I'd just want to think really hard and see if there's any way aeson can encode a Map or HashMap to have undefined values, in a way that isn't due to the value itself encoding to undefined...

[thomasjm]

(Is there some situation where k: string and k in string differ in TypeScript?)

[wraithm]

The commit comment says:

This allows to have unions as keys, which often happens when encoding variants made out of nullary constructors.

It sounds like string keys shouldn't ever cause this. I agree that it'd be great to not have the ? for string keys. Or maybe more generally, for types where there's no possibility of undefined.

[thomasjm]

Having just closed #56, I have some more context now on the tradeoff here. Going back to the original comment:

It's a bit difficult to use these types, since indexing IExample will give a number | undefined value, even though the ToJSON instance for Data.Map should never include undefined values.

The issue isn't (just) that the map would contain an explicit undefined value. What this is protecting you from in your TypeScript code is if you should index into your map with a string that may not exist as a key. I.e.

const foo: Example = { bar: 42 }
console.log(foo["baz"] + 2)

In the above, foo["baz"] will be typed as number | undefined, forcing you to handle the case where the key didn't exist.

I agree that this is cumbersome. For example, if you use map from lodash like this:

map(foo, (n, k) => ...)

Then n will still be typed as number | undefined, which would never actually happen. IMO the indexed type syntax in TypeScript is actually kind of flawed because it conflates the missing key case with the possibility of an explicit undefined in the map.

So here's where I've come down on this:

• In general, we definitely want the question mark, because there is no guarantee that Data.Map will be total.
• For keys that have an infinite range of possible values, like number or string, I think it's debatable. Since you already know it's impossible for the map to include every value, it might be more idiomatic to just omit the question mark. Or, we could even go back to a index signature rather than a mapped type (i.e. { [k: string]: number }. But I think we would want to special case this for these particular key types.

CC @jkachmar since this arises from the course we've taken in #55, #56.