Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
benrbray opened this issue Jul 21, 2023 · 3 comments

Comments

@benrbray
Copy link

benrbray commented Jul 21, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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

@wraithm
Copy link

wraithm commented Apr 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants