Thoth.Json icon indicating copy to clipboard operation
Thoth.Json copied to clipboard

Set of features for Thoth.Json v5

Open MangelMaxime opened this issue 5 years ago • 14 comments

  • Move the path construction in the Error case only
  • Make Thoth.Json(.Core) parser agnostic. The idea is to have all the decoder/encoder logic write once, and have a contract to fill in order to change the parser. Like that people could use Newtonsoft, JavaScript native API, their own parser, Parsimmon, etc. depending on their preferences
  • Force the encoder/decoder to be in sync
  • Allow the user to override the standard decoder. For example, perhaps they want to represents the DateTime in a specific manner

MangelMaxime avatar Feb 10 '20 19:02 MangelMaxime

Should we add the possibility to support different representation for the types? (see #57)

It should already be possible to use "override of the standard decoder" with a Decode.oneOf in it, but we can/want to provide a cleaner/easier API for this case.

MangelMaxime avatar Feb 12 '20 12:02 MangelMaxime

Add a way to not crash the encoder/decoding process if no coder is found a type. This is need for Elmish.Debugger, because people can store anything in their model and it's not really friendly to ask them to provide encoders/decoders for using a dev-tool. (see #59)

Make a strong note in the documentation that this feature is really risky because it will weaken Thoth.Json security when this flag is active.

MangelMaxime avatar Feb 26 '20 09:02 MangelMaxime

+1 for not unifying the parser into one. I had issues where JSON parsers implemented on the JS side were not as performant as the native JSON.parse.

njlr avatar Mar 05 '20 15:03 njlr

@njlr It will never be as performant as the native JSON.parse because we are calling it and then checking the validity of the JSON data.

MangelMaxime avatar Mar 05 '20 16:03 MangelMaxime

Today I read this article and it made me think once again about how we deal with JSON.


I never was a big fan of the Auto modules.

The implementation is hard to maintain and people want complete control over what it is generating by customizing just the one things they don't like by default or which needs to be deal depending on the case.

Auto modules are also necessary slower than manual decoder. Indeed manual decoder just does the action needed for the conversion while auto coders have a lot of condition, use reflection, etc. before doing the conversion.

Unfortunately, people seem to prefer the auto decoders compared to the manual decoder because it is hiding the complexity and/or the boilerplate code. At least, it was the reason that leads to its creation.

I think I will remove the auto module in a next release probably the one where I refactor the whole package by trying to mutualize the code etc.

If I am to remove it I do agree that writing the coders manually is not fun. For this reason, I will seriously explore the code generation that I left on the side for a long time.

Code generation will allow people to not write "dummy" manual coders when the default generation is doing the job for them. And if they need to customize it then they can deactivate the code generation and copy/paste/modify the generated code to fit their needs.

MangelMaxime avatar May 08 '20 19:05 MangelMaxime

I wanted to just briefly weigh in on this, as I am currently using both approaches for different reasons.

For serializing to/from persistent stores, the manual encoders are the only way imo. They give you total control, and this has enabled us to use a versioning system so that historic json payloads with different shapes can be automatically transposed onto the new model structure, saving the need to mess around running data migrations and all that pain.

However, going from server to client, we have quite a complicated domain model, and using Auto coders is an extremely easy way to pass symmetrical payloads to and from the client without doing any grunt work. This is especially handy when you have shared request/response typed payloads that are forever changing. I totally appreciate that if one of the two goes out of sync, your going to get catastrophic explosions, but our release cadence mandates that the client always goes out with the server, so this is just not a problem for us (and in my experience this is not uncommon unless microservices).

It would be a shame to entirely loose Auto coders, even though they are probably abused far too often. Perhaps it could exist in a standalone package?

alexswan10k avatar May 13 '20 11:05 alexswan10k

Thank you for sharing @alexswan10k

And I do agree, if auto-coders are to be kept during the big rewrite they will have to go in a separate package.

Edit:

I think I need to accept that auto coders are a thing and it's something which is selling the library. But I need to re-think the implementation, set of features and also the documentation in order to explain why/when to use them and also what is supported so people know exactly what they can do with it.

MangelMaxime avatar May 16 '20 15:05 MangelMaxime

Today, I started prototyping the next version of Thoth.Json.

I wanted to check if my idea of using an interface to abstract the implementation details about which JSON parser is being used could work. And it seems like yes 🎉 🎉

Here are the implementations for the Decode modules:

As we can see the code required to add a new parser is quite small :)

My theory is that we will have this architecture:

  • Thoth.Json.Core -> Library which provide and use the abstract interface IDecoderHelpers<'JsonValue> to implement the logic to hand JSON manipulation
  • Thoth.Json.Fable | Thoth.Json.Newtonsoft | Thoth.Json.XXX -> Libraries which implements the interface IDecoderHelpers<'JsonValue> using a specific framework and also provides the "runners" Decode.fromString, Decode.unsafeFromString and Decode.fromValue.

The next step for me is to mutualize the tests between the different implementations because right now, I duplicated them locally for testing purpose. Then, I will take the time to re-activate all the decoders because I disabled some of them as they were a bit more complex to port and I preferred to focus on building a small prototype first.

Mutualizing the tests should show us how it will look like consuming Thoth.Json from different parsers. In theory, we should not see anymore the compiler directives :D

I am also introducing another repository called https://github.com/thoth-org/Thoth.Json.Meta which aims to use submodules in order to keep each project in their own repository but still allow them to be sync.

I think it will be more generic to the whole Thoth ecosystem at some point because Thoth.Fetch could probably be added too I think. The idea behind this "meta" repository is to solve the problem of sharing the code/tests/etc. between the different implementation.

MangelMaxime avatar May 17 '20 19:05 MangelMaxime

Hum, I am facing a problem...

I had to use a generic 'JsonValue in order to make the implementation parser agnostic but the generic need to propagate in the outer scope.

This lead to code like that:

let levelOfAwesomenessDecoder<'JsonValue> =
    Decode.string<'JsonValue>
    |> Decode.andThen (fun value ->
        match value with
        | "MaxiAwesome" -> Decode.succeed MaxiAwesome
        | "MegaAwesome" -> Decode.succeed MegaAwesome
        | _ ->
            sprintf "`%s` is an invalid value for LevelOfAwesomeness" value
            |> Decode.fail
    )

Otherwise, the decoder cannot be used when using ProjectRefence it would works if people use file reference but that's not really idiomatic.

And this code doesn't work (at least I don't know how to make the generic propagate correctly):

image

A solution could be to not use module/function to declare Decode and Encode feature but use classes. However, I think the code will look to:

let levelOfAwesomenessDecoder<'JsonValue> =
    Decode<_>.string
    |> Decode<_>.andThen (fun value ->
        match value with
        | "MaxiAwesome" -> Decode.succeed MaxiAwesome
        | "MegaAwesome" -> Decode.succeed MegaAwesome
        | _ ->
            sprintf "`%s` is an invalid value for LevelOfAwesomeness" value
            |> Decode.fail
    )

Which is really ugly

I think I need to explore new ideas in order to make the implementation parser agnostic and cross-platform.

MangelMaxime avatar May 29 '20 20:05 MangelMaxime

New idea:

Disclaimer: I am writing my though down because it helps me shape my ideas and not forget, so perhaps it is not always well explain sorry about that. As soon, as I have something working I would make a proper post to ask feedback etc. as I do in general.

I think my current rewrite is trying to solve several things at the same times:

  1. Make contribution easier especially for the tests of Thoth.Json.Net -> Use a meta-repository with submodules to organise the project structure

  2. Mutualize the code between the different implementation of Thoth.Json -> I tried using generic because it was looking like the solution but it doesn't work out

  3. Make it easier to write parser agnostic code -> Related to point 2

  4. Improve the performance by moving the path construction on the error path and not success case -> This is just an implementation detail and it is working in my local repo

  5. Try to improve the user experience by removing the compiler directives -> Related to point 2

The goal of using generics was to have a contract :

type IDecoderHelpers<'JsonValue> =
    abstract GetField : FieldName : string -> value : 'JsonValue -> 'JsonValue
    abstract IsString : value : 'JsonValue -> bool
    abstract IsBoolean : value : 'JsonValue -> bool
    abstract IsNumber : value : 'JsonValue -> bool
    abstract IsArray : value : 'JsonValue -> bool
    abstract IsObject : value : 'JsonValue -> bool
    abstract IsNullValue : value : 'JsonValue -> bool
    abstract IsIntegralValue : value : 'JsonValue -> bool
    abstract IsUndefined : value : 'JsonValue -> bool
    abstract AnyToString : value : 'JsonValue -> string

    abstract ObjectKeys : value : 'JsonValue -> string seq
    abstract AsBool : value : 'JsonValue -> bool
    abstract AsInt : value : 'JsonValue -> int
    abstract AsFloat : value : 'JsonValue -> float
    abstract AsFloat32 : value : 'JsonValue -> float32
    abstract AsString : value : 'JsonValue -> string
    abstract AsArray : value : 'JsonValue -> 'JsonValue[]

were each implementation could implement the contract. The problem as explained in my previous message is that the generics are escaping the bound of the "parser related library".

If we think in term of macro generic is just a feature of the compiler which allows us to re-use code. In my case, the goal was to have Thoth.Json.Core, Thoth.Json.Fable, Thoth.Json.Newtonsoft, etc.

What if we were going back a bit and integrate directly Thoth.Json.Core into each of the library-specific implementation.

The idea would be in that case to share the code not by using generics code but by referencing directly the implementation files and just having the "contract" written in another file.

Something like that:

<!-- Thoth.Json.Fable -->
<?xml version="1.0" encoding="utf-8"?>
<Project Sdk="Microsoft.NET.Sdk">
  <ItemGroup>
    <Compile Include="Contract.fs" /> <!-- The file would probably be called Helpers.fs -->
    <Compile Include="Thoth.Json.Core/Shared/Decode.fs" />
    <Compile Include="Thoth.Json.Core/Shared/Encode.fs" />
  </ItemGroup>
</Project>

<!-- Thoth.Json.Newtonsoft -->
<?xml version="1.0" encoding="utf-8"?>
<Project Sdk="Microsoft.NET.Sdk">
  <ItemGroup>
    <Compile Include="Contract.fs" /> <!-- The file would probably be called Helpers.fs -->
    <Compile Include="Thoth.Json.Core/Shared/Decode.fs" />
    <Compile Include="Thoth.Json.Core/Shared/Encode.fs" />
  </ItemGroup>
</Project>

The idea is that files under Thoth.Json.Core/Shared/* are re-used directly inside the different library. The end-user would still need to use compiler directives in it's shared code but at least the implementation would be the same for each specific library.

We could always use a few compiler directives inside the shared implementation when there are runtime specific requirements:

image

For example, in order to support enum on Fable runtime, we need to inline the decoder in order to get the type information used for the reflection.

This approach is similar to how it is down today. But today, we kind of copy/paste the code between the repo and adapt the code in Thoth.Json.Net especially for Auto module, the goal would be to have as much as possible the same implementation so adding new parser support would "just" be about implementing the Contract.fs file and releasing a new package (and of course running the tests etc.)

MangelMaxime avatar May 29 '20 20:05 MangelMaxime

Work for the next version of Thoth.Json has started in #76

The goal of this work is to mutualize as much as possible the code between the different implementation. Right now, I have ported the code for:

  • Thoth.Json.Fable -> this is using the native JSON.Parse API
  • Thoth.Json.Newtonsoft -> equivalient of Thoth.Json.Net which use Newtonsoft API

The goal of mutualizing the code is that it should be possible to add a new target like JSON.Net if someone prefers this library or for performance reason.

These packages are already working and follows the same philosophy as before for sharing the code. User need to use compiler directives currently THOTH_JSON_FABLE and THOTH_JSON_NEWTONSOFT.

Current thought:

One goal of the new rewrite was also to make it easier to share code, for this reason, I plan on converting the package Thoth.Json to be platform-independent #38 so people can share code using NuGet and avoid compiler directives.

However, the performance will probably not be as good as when using native JSON.Parse or Newtonsoft or any performance specialized JSON parser. This one will be to provide easy to use code sharing.

If people really need highly performant parser they will need to use dedicated package using compiler directives. I think that for most users it should be ok to use Thoth.Json.

MangelMaxime avatar Jul 01 '20 09:07 MangelMaxime