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
151 stars 34 forks source link

Use own parser to fuse Thoth.Json and Thoth.Json.Net #38

Open MangelMaxime opened 4 years ago

MangelMaxime commented 4 years ago

Issue by alfonsogarciacaro Sunday Feb 24, 2019 at 23:21 GMT Originally opened as https://github.com/MangelMaxime/Thoth/issues/128


Now that Thoth.Json 3 beta is published it's time to think of v4 😸

It's great that Thoth can be used both in JS and .NET. However, the fact that there're are two packages for this complicates maintenance and confuses users, as other cross-platform packages only have one published version (see #124).

AFAIK. the only reason for the two packages is to use JSON.parse in JS and Newtonsoft.Json in .NET. This seems sensible because parsing JSON looks complicated. However, this JSON parser by Jon Harrop is just a few lines of code and it's compatible with Fable. We can adapt it and make it a bit more performant (don't split the string, etc).

Also, if we control the DSL we don't need so many helpers (isNumber, isNull...) and transforming to string after encoding shouldn't be too difficult either.

What do you think @MangelMaxime?

MangelMaxime commented 4 years ago

Comment by MangelMaxime Monday Feb 25, 2019 at 07:44 GMT


We not only need a parser but a writer too.

We also need to see how performant it is for big JSON parsing/writing. Thoth.Json v3 isn't yet released in stable version so it's should be possible to include the parser change in it.

And of course, I would love to see Thoth.Json.Net deprecated and make user life simpler :) This is probably the last big issues with Thoth.Json at the moment.

~I will give it a try today and see how it goes. Will keep inform :)~

Edit: I will explore this possibilities after finishing the repo rework. Like Fulma, this repo will be splitted in several repos and I have already started the work locally so I can't easily contribute to the old version.

MangelMaxime commented 4 years ago

Comment by danyx23 Friday Mar 01, 2019 at 09:58 GMT


I think unifying the JS and .NET Thoth stuff would be great. We have moved away from the recommended #ifdef solution since it confused ionide and didn't play well with nuget packages that were to be used both by fable and .net projects.

My personal thinking (first roughly expressed in #45) is to create a new nuget package that contains only interface definitions for an IDecoder<'jsonType> and IEncoder<'jsonType> that can parse primitive values and is generic over the json implementation and then have concrete implementations (that also provide a fromString and toString respectively) for the different underlying json library.

So roughly:

type IJsonDecodeProvider<'JsonType> =
  abstract isNumber : 'JsonType-> bool
  abstract asFloat : 'JsonType-> float
  // ..

type Decoder <'T, 'JsonType> = IJsonDecodeProvider<'JsonType> -> string -> 'JsonType -> Result<'T, DecoderError<'JsonType>>

And then e.g. the newtonsoft implementation would be

open Newtonsoft.Json.Linq
let NewtonsoftJsonProvider =
  { new IJsonDecodeProvider<JToken> =
      member this.isNumber (token : JToken) = token.Type = JTokenType.Float || token.Type = JTokenType.Integer
      // ...
  }

let fromString = ...

I wanted to try this out in a fork for a long time but haven't gotten around to it yet. The nice thing would be that if all you want to do is declare Encoders/Decoders you only need to reference the library that declares the interfaces, only when you want to actually serialize/deserialize do you have to decide on a concrete library

MangelMaxime commented 4 years ago

Comment by MangelMaxime Friday Mar 01, 2019 at 10:20 GMT


So you would expose Decode.fromString, Decode.Auto.fromString, Encode.toString, Encode.Auto.ToString only from the "implementation" packages?

MangelMaxime commented 4 years ago

Comment by danyx23 Friday Mar 01, 2019 at 10:27 GMT


That was my Idea, yes (but I don't use Auto decoders so can't comment on how feasibly/desirable that is for this). Maybe there is a good reason to add this to the interface as well, but it looks to me like this is not necessary for the base library to already be useful for declaring coders in libraries

MangelMaxime commented 4 years ago

Comment by MangelMaxime Friday Mar 01, 2019 at 10:42 GMT


I will try to play with both of your ideas and see how to goes.

And nothing prevents a mix of both . Using a "contracts" library and showing that you can use any JSON parser in it even yours for whatever reason (slim dependencies perhaps).

MangelMaxime commented 4 years ago

Comment by kspeakman Friday Mar 15, 2019 at 22:32 GMT


I've been exploring using a single serializer for both sides, but I think our team is going to let that dream die. Some things just don't make sense to deal with in client-side codecs because JS can't handle them natively anyway. For example, server-side may store json in database. So its serializer could write/read high-precision decimal properties as a JSON number no problem. But sending the same to the client, I would create a new type with the property as a string for display. Or if it is money, an integer number of cents for calculations.

We use auto-codecs almost exclusively. In our situation parsing flexibility is not used, so maintaining manual codecs has extra cost with zero benefit.