thoth-org / Thoth.Json

Library for working with JSON in a type safe manner, this libs is targeting Fable
https://thoth-org.github.io/Thoth.Json/
MIT License
150 stars 36 forks source link

`Decode.fromString Decode.value ""` returns `Ok null` instead of an error #185

Closed njlr closed 5 months ago

njlr commented 10 months ago

Decode.fromString Decode.value "" returns Ok null instead of an error.

Issue is demonstrated by this script:

#r "nuget: Thoth.Json.Net, 11.0.0"

open Thoth.Json.Net

let tryNormalizeJson (maybeJson : string) : string =
  match Decode.fromString Decode.value maybeJson with
  | Ok j -> Encode.toString 2 j
  | Error _ -> maybeJson

let myJson = ""
let normalized = tryNormalizeJson myJson

printfn $"%s{normalized}"

(*

System.NullReferenceException: Object reference not set to an instance of an object.
   at Thoth.Json.Net.Encode.toString(Int32 space, JToken token)
   at <StartupCode$FSI_0002>.$FSI_0002.main@() in /home/runner/Scratch.fsx:line 12
Stopped due to error

*)
MangelMaxime commented 10 months ago

Hum, this is because Decode.value does no checks.

It is defined as let value _ v = Ok v meaning that anything you pass to it succeed. It was added because Elm implementation had it.

So I am unsure what to do here. TBH honest I never used Decode.value 🙈

MangelMaxime commented 5 months ago

Looking once again at how Elm implements this I believe the current implementation of Decode.value is correct in the legacy package.

Because, I don't know in which case Decode.value should be used, I am going to remove it from the new API.

It implementation would looks like this:

let value<'a> =
    { new Decoder<'a> with
        member _.Decode(_, value) = Ok (unbox value)
    }

let succeed (output: 'a) : Decoder<'a> =
    { new Decoder<'a> with
        member _.Decode(_, _) = Ok output
    }

Which feels really strange to me and I believe the goal of Decode.value in Elm lang, is to work around some interop things if needed. If someone, is using it please feel free to open an issue with the use case so we can check if we should support it or if there are others API to archive te same things.

@njlr If you want me to look into your use case, please feel free to provide a example with an explanation of what you are trying to archive.