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

Work on the unification of Thoth.Json and Thoth.Json.Net #175

Open MangelMaxime opened 1 year ago

MangelMaxime commented 1 year ago

Nowadays, I leaning to having a Thoth.Json package which contains the logic of the decoders and then have different backend package which implements an interface to provide support for Fable.Core.JS.JSON, Newtonsoft, System.Text.Json, etc.

njlr commented 11 months ago

This sounds great! Please let me know if any help is required. I have an almost complete implementation of the Auto module (reflection) that may help.

MangelMaxime commented 11 months ago

Thank you for proposing.

Currently, I have the manual API done for both encoder and decoder. And will definitely need help and feedback on the implementation.

For example, my plan is to make Thoth.Json package obsolete and make people use:

My idea by making Thoth.Json obsolete is I can inform the user of the new library structure. And I believe having Thoth.Json.Core does emphase that this is just the library that can be shared between runtime.

njlr commented 11 months ago

make Thoth.Json package obsolete

I think this makes sense so that existing users can stay on the old approach.

Thoth.Json.Newtonsoft (should it be named Thoth.Json.DotNet.Newtonsoft?) Thoth.Json.System.Text.Json (should it be named Thoth.Json.DotNet.System.Text.Json?)

My view is the shorter names are better - and a JS version of these seems unlikely!

In terms of project structure, one of the pain-points before was having to work across two repos. This will of course become worse with more "back-ends". Any thoughts on maintaining the official packages in one place?

With AoT compilation becoming more important, I think the "auto" module should also be spun out of the core. This means that reflection is opt-in, and allows more to be built on top of Thoth.Json.Core.

MangelMaxime commented 11 months ago

My view is the shorter names are better - and a JS version of these seems unlikely!

Well it is not only JavaScript but also Python, Rust, and any future target that Fable will support.

But I do agree, that the chance of having a conflict in the name of the libraries is very low. TBH currently, I am using the short version name for the packages.

In terms of project structure, one of the pain-points before was having to work across two repos. This will of course become worse with more "back-ends". Any thoughts on maintaining the official packages in one place?

There are indeed all going under the same repository.

CleanShot 2023-11-17 at 21 14 51

This is a bit of a mess right now, but this idea is you have the different libraries under packages and inside of tests you have the tests source file and 1 project per target (JS, Newtonsoft, etc.) which consume these sources files.

The idea is that we want to write the exact same source code for each target to make sure that they support the same features sets. Some different can exists for things that are platform specific like the invalid JSON exception but 99% of tests should use the same code.

With AoT compilation becoming more important, I think the "auto" module should also be spun out of the core. This means that reflection is opt-in, and allows more to be built on top of Thoth.Json.Core.

If we are to move it to a separate package it probably means that we will need to duplicate the number of packages.

Personally, I am ok with moving it separate packages because it will show that this is an opt-in feature. And this means that features like CEs DSLs, codec, will be treated in the same way. They are additions to the manual API.

njlr commented 7 months ago

Is there a WIP branch for this?

MangelMaxime commented 7 months ago

Hello @njlr,

The manual APIs packages have already been created and pushed to NuGet.

To use the new API, you need to open Thoth.Json.Core to access the decoders API.

And then open Thoth.Json.JavaScript for example to in your JavaScript project.

I was working on updating the documentation before making an announcement and as always had to prioritise other stuff (and was kind of expecting Eleventy v3 to be released soon but it has not been the case 😅). Should have some time available in the coming weeks to finish that and have it off my mind.

I think I have a local branch to work on the Auto API but I will need to take a look to see in what state it is currently in.

njlr commented 7 months ago

Thanks, I was able to play around with it:

#r "nuget: Thoth.Json.Core, 0.2.1"
#r "nuget: Thoth.Json.Newtonsoft, 0.1.0"

open Thoth.Json.Core

type Book =
  {
    Title : string
    Year : int
    Author : string
  }

[<RequireQualifiedAccess>]
module Book =

  let encode : Encoder<Book> =
    fun x ->
      Encode.object
        [
          "title", Encode.string x.Title
          "year", Encode.int x.Year
          "author", Encode.string x.Author
        ]

  let decode : Decoder<Book> =
    Decode.object
      (fun get ->
        {
          Title = get.Required.Field "title" Decode.string
          Year = get.Required.Field "year" Decode.int
          Author = get.Required.Field "author" Decode.string
        })

open Thoth.Json.Newtonsoft

[<RequireQualifiedAccess>]
module Decode =

  let unsafeFromString decoder json =
    match Decode.fromString decoder json with
    | Ok x -> x
    | Error error -> failwith error

let book =
  {
    Title = "The Monkey Wrench Gang"
    Year = 1975
    Author = "Edward Abbey"
  }

let json =
  book
  |> Book.encode
  |> Encode.toString 2

printfn $"%s{json}"

let decoded =
  json
  |> Decode.unsafeFromString Book.decode

printfn $"%A{decoded}"

One suggestion to make upgrading easier would be for the Core module be [<AutoOpen>]?

njlr commented 7 months ago

Maybe I hit a bug?

#r "nuget: Thoth.Json.Core, 0.2.1"
#r "nuget: Thoth.Json.Newtonsoft, 0.1.0"

open Thoth.Json.Core
open Thoth.Json.Newtonsoft

let json =
  Encode.int16 -7s
  |> Encode.toString 0

printfn $"%s{json}"

// 4294967289
njlr commented 7 months ago

I have tidied up the auto code I have and ported it to Thoth.Json.Core: https://github.com/njlr/thoth-json-auto

It largely unifies Fable and .NET, aside from cases where Fable's reflection capabilities are limited (here erasure can be used instead).

If it's useful, I can help integrate thoth-org.

Thanks!

MangelMaxime commented 7 months ago

Maybe I hit a bug?

#r "nuget: Thoth.Json.Core, 0.2.1"
#r "nuget: Thoth.Json.Newtonsoft, 0.1.0"

open Thoth.Json.Core
open Thoth.Json.Newtonsoft

let json =
  Encode.int16 -7s
  |> Encode.toString 0

printfn $"%s{json}"

// 4294967289

Perhaps, F#/Newtonsoft doesn't like all the type casting happening?

https://github.com/thoth-org/Thoth.Json/blob/606f88582f3eb66dfacc2afd226f7b9c73a417cc/packages/Thoth.Json.Core/Encode.fs#L41

https://github.com/thoth-org/Thoth.Json/blob/606f88582f3eb66dfacc2afd226f7b9c73a417cc/packages/Thoth.Json.Newtonsoft/Encode.fs#L33-L37

Thoth.Json.Core unify all the integral values to uint32 to respect the old Thoth.Json behaviour. Values bigger than max uint32 are encoded as a string and not a number in JSON.


Thanks for looking into the Auto API, I will have a look at it.

njlr commented 7 months ago

Perhaps this is a good time to make a breaking change?

#r "nuget: System.Text.Json, 8.0.3"

open System.Text.Json

let json = JsonSerializer.Serialize(-7s)

printfn $"%s{json}"

// -7

I don't think users would expect overflow for Thoth.Json.System.Text.Json (needs a better name! :smile: )

MangelMaxime commented 7 months ago

Oh you mean, I made a mistakes by converting a int16 into an unsigned int which don't accept negative values?

Depends on what you mean by breaking changes, the JSON representation cannot be changed compared to Thoth.Json because I know some people who stored the JSON databases meaning we need to be backward compatible at least for the JSON representation. API wise, it is possible to have breaking changes indeed Thoth.Json.Core is released as 0.x which is a beta version.

njlr commented 7 months ago

Oh you mean, I made a mistakes by converting a int16 into an unsigned int which don't accept negative values?

Depends on what you mean by breaking changes, the JSON representation cannot be changed compared to Thoth.Json because I know some people who stored the JSON databases meaning we need to be backward compatible at least for the JSON representation. API wise, it is possible to have breaking changes indeed Thoth.Json.Core is released as 0.x which is a beta version.

On this type:

[<RequireQualifiedAccess; NoComparison>]
type Json =
    | String of string
    | Char of char
    | DecimalNumber of float
    | Null
    | Boolean of bool
    | Object of (string * Json) seq
    | Array of Json[]
    // Thoth.Json as an abritrary limit to the size of numbers
    | IntegralNumber of uint32
    | Unit

I wonder where a negative int32 e.g. -7 should live?

It's interesting to see how other libraries handle this.

yojson allows for an int32 or a float: https://github.com/ocaml-community/yojson/blob/4c1d4b52f9e87a4bd3b7f26111e8a4976c1e8676/lib/type.ml#L3-L33

FSharp.Data uses decimal or float: https://github.com/fsprojects/FSharp.Data/blob/674cacf816f16acc4dd9c9305a89666c05064801/src/FSharp.Data.Json.Core/JsonValue.fs#L37-L46

Newtonsoft has JTokenType.Integer, which is used to represent int32, int64, ulong, ... https://github.com/JamesNK/Newtonsoft.Json/blob/55a7204f9b9546aa07145591d42046d509176ad4/Src/Newtonsoft.Json/Linq/JValue.cs#L105-L113

The JSON spec leaves it up to the implementation :shrug:

njlr commented 7 months ago

I wonder if Encoder<'T> should also be an interface that accepts a JSON representation type argument.

Before:

type Encoder<'T> = 'T -> Json

After:

type IEncodable =
    abstract member Encode<'JsonValue> :
        helpers: IEncoderHelpers<'JsonValue> -> 'JsonValue

type Encoder<'T> = 'T -> IEncodable

// Probably won't work as-is, just a sketch

The Json type could exist for fancy cases Decode.value. It is useful when you want to do "functional programming" type stuff on JSON objects; being a simple F# type, Json works nicely with match, Map etc.

Worth noting that the JSON type for encoding and decoding may be different (see https://github.com/thoth-org/Thoth.Json.Net/issues/52#issuecomment-1515249737)

MangelMaxime commented 7 months ago

I wonder if Encoder<'T> should also be an interface that accepts a JSON representation type argument.

I don't remember the exact problem but I was not able to do it because of how the generics types are resolved with F#. I think, the generics leaked too far in the caller code leading to issues / code being not friendly.

Sorry, I don't exactly remember the situation it was a few months ago.

Using the current Json domain have some benefit, it enforce the arbitrary rules that Thoth.Json have regarding number representation and also it helps the user write the correct code when dealing with Array, List, etc.

The compiler now helps him identify that he needs to call List.map, etc.

Worth noting that the JSON type for encoding and decoding may be different (see https://github.com/thoth-org/Thoth.Json.Net/issues/52#issuecomment-1515249737)

This should not be a problem because IDecoderHelpers<'JsonValue> and IEncoderHelpers<'JsonValue> and 2 distincts interfaces. Yes they both use 'JsonValue but this is just an alias name it doesn't enforce anything.

I will just need to remember this fact when creating Thoth.Json.Core.Codec because then this interface will look something like that:

type ICodecHelpers<'DecodeJsonValue, 'EncodeJsonValue> =
    inherit IDecoderHelpers<'DecodeJsonValue>
    inherit IEncoderHelpers<'EncodeJsonValue>

// or

type ICodecHelpers<'DecodeJsonValue, 'EncodeJsonValue> =
    abstract decoderHelpers: IDecoderHelpers<'DecodeJsonValue>
    abstract encoderHelpers: IEncoderHelpers<'EncodeJsonValue>

And so here we need to make the distinction between both generic types.

njlr commented 7 months ago

I wonder if Encoder<'T> should also be an interface that accepts a JSON representation type argument.

I don't remember the exact problem but I was not able to do it because of how the generics types are resolved with F#. I think, the generics leaked too far in the caller code leading to issues / code being not friendly.

Sorry, I don't exactly remember the situation it was a few months ago.

I think the trick is to have two types Encoder and IEncodable (name could be IJson?) to prevent the generic from leaking.

I made a draft PR to show what I mean here: https://github.com/thoth-org/Thoth.Json/pull/188

It's quite a small impact change overall.

njlr commented 6 months ago

Here is an implementation of "Auto" that is agnostic to the JSON backend: https://github.com/thoth-org/Thoth.Json/pull/189

MangelMaxime commented 6 months ago

@njlr Thank you I would make some time during this week to look at all the PRs you sent.

The one reworking the manual API looked good last time I checked and will probably be released this week. For the Auto API, I need to look into more as this is a more tricky API IHMO and also because there are improvements that I want to add compared to the legacy behaviour.

I will also take the time to make yet another macro list of what I would like to have done before considering the new Thoth.Json "stable" in order to push the that stable release forward.

Thank you again for all the time you are investing in this project.

njlr commented 6 months ago

@njlr Thank you I would make some time during this week to look at all the PRs you sent.

The one reworking the manual API looked good last time I checked and will probably be released this week. For the Auto API, I need to look into more as this is a more tricky API IHMO and also because there are improvements that I want to add compared to the legacy behaviour.

I will also take the time to make yet another macro list of what I would like to have done before considering the new Thoth.Json "stable" in order to push the that stable release forward.

Thank you again for all the time you are investing in this project.

That sounds good!

Next on my list was a System JSON backend, but will wait for your list to avoid overlap.