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

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

Open
MangelMaxime opened this issue Apr 18, 2023 · 19 comments
Open

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

MangelMaxime opened this issue Apr 18, 2023 · 19 comments

Comments

@MangelMaxime
Copy link
Contributor

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

njlr commented Nov 16, 2023

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

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:

  • Thoth.Json.Core
  • Thoth.Json.JavaScript
  • 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?)
  • Thoth.Json.Python.Orjson

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

njlr commented Nov 17, 2023

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

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.

  • Thoth.Json.Core
  • Thoth.Json.JavaScript
  • Thoth.Json.Auto.Core
  • Thoth.Json.Auto.JavaScript

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

njlr commented Mar 29, 2024

Is there a WIP branch for this?

@MangelMaxime
Copy link
Contributor Author

Hello @njlr,

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

  • Thoth.Json.Core
  • Thoth.Json.JavaScript
  • Thoth.Json.Python
  • Thoth.Json.Newtonsoft

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

njlr commented Mar 29, 2024

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

njlr commented Mar 29, 2024

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

njlr commented Mar 30, 2024

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

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?

let inline int16 (value: int16) = Json.IntegralNumber(uint32 value)

member _.encodeIntegralNumber(value: uint32) =
// We need to force the cast to uint64 here, otherwise
// Newtonsoft resolve the constructor to JValue(decimal)
// when we actually want to output a number without decimals
JValue(uint64 value)

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

njlr commented Mar 30, 2024

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! 😄 )

@MangelMaxime
Copy link
Contributor Author

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

njlr commented Mar 30, 2024

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?

  • IntegralNumber loses the sign
  • DecimalNumber loses the fact it is a whole number

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 🤷

@njlr
Copy link
Contributor

njlr commented Mar 30, 2024

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
  • This would avoid the overhead of constructing Json objects in some cases
  • It allows the backend to have more control over how encoding works - it doesn't have to go through the Json representation, which may lose information

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 thoth-org/Thoth.Json.Net#52 (comment))

@MangelMaxime
Copy link
Contributor Author

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 thoth-org/Thoth.Json.Net#52 (comment))

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

njlr commented Mar 30, 2024

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: #188

It's quite a small impact change overall.

@njlr
Copy link
Contributor

njlr commented Apr 14, 2024

Here is an implementation of "Auto" that is agnostic to the JSON backend: #189

@MangelMaxime
Copy link
Contributor Author

MangelMaxime commented Apr 15, 2024

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

njlr commented Apr 15, 2024

@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.

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

2 participants