codedownio / aeson-typescript

Generate TypeScript definition files from your ADTs
BSD 3-Clause "New" or "Revised" License
60 stars 27 forks source link

Extend Map instance to handle `ToJSONKeyValue` keys #28

Closed Ptival closed 1 month ago

Ptival commented 2 years ago

This code:

https://github.com/codedownio/aeson-typescript/blob/16d6c2a40730fdc72784ca70391b68c5086d78a1/src/Data/Aeson/TypeScript/Instances.hs#L114-L116

seems to assume aeson generates maps for Data.Map.

This does not appear to be the case. Since JavaScript maps must be keyed by strings, aeson instead generates key-value pairs, see:

https://github.com/haskell/aeson/blob/65fca856304c0b323d538cc783493d8d7a145114/src/Data/Aeson/Types/ToJSON.hs#L1684

and the discussions at:

https://github.com/haskell/aeson/issues/259 https://github.com/haskell/aeson/issues/79

It seems aeson-typescript should instead generate key-value pairs.

thomasjm commented 2 years ago

It certainly encodes to a JSON object for every case I'm aware of. Those are some pretty old issues also. Do you have an example of this?

thomasjm commented 2 years ago

Oh, I guess this can happen when you use a ToJSONKeyValue function, see https://hackage.haskell.org/package/aeson-2.0.3.0/docs/Data-Aeson-Types.html#t:ToJSONKey

Ptival commented 2 years ago

Ah indeed, in our example we have a:

newtype Name (sym :: Symbol) = Name { name :: Text }
  deriving (Eq, Ord, Show, Generic, NFData)

that has a ToJSONKey instance and is used as the key of a Map Name ...!

thomasjm commented 2 years ago

Yep, sounds like we need to modify the instance to handle this case. Wouldn't you rather just use ToJSONKeyText? IIRC you can even auto derive it from a newtype like this.

Ptival commented 2 years ago

Sorry I mistyped, just a ToJSONKey* instance. I don't think we call either of toJSONKeyValue or toJSONKeyText explicitly, but I'm not certain as I did not write those instances.