Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
273 stars 55 forks source link

Q: Ending up with two libraries to serialize things. #80

Closed nojaf closed 6 years ago

nojaf commented 6 years ago

Hi, I'm using Fable.Remoting.Client and I noticed it has a dependency on Fable.PowerPack and Fable.SimpleJson. If you look closer Fable.PowerPack has a dependency on Thoth.Json.

So I guess in my bundle I end up with two libraries that serialize json. I'm having some mixed feelings about this. I'd like to hear your thoughts on this.

//cc @MangelMaxime

Zaid-Ajaj commented 6 years ago

Hello @nojaf, I am well aware of this issue, remoting client uses power-pack for easily working with promises and fetch api. I was thinking in breaking power pack down to seperate libraries (fetch + promise) and use only what I need. However, I am not sure whether this is a problem because the code in Thoth.Json isn’t used and is probably eliminated but I need to confirm this

MangelMaxime commented 6 years ago

I think like Zaid, if Fable.Remotong do not use the code touch by Thoth.Json then it should not have it in the generated bundle. Just because Fable will not import it.

One problem in breaking PowerPack into several libraries is that it's already used at a lot of place. So yes, we could indeed add Obselete attribute so people know that they need to use Fable.PowerPack.Fetch, Fable.PowerPack.Promise, etc. but I am not sure if this is worth it.

Also, Fable.PowerPack.Fetch would still probably use Thoth.Json internally or we would need Fable.PowerPack.Fetch.Thoth.Json or Thoth.Fetch to isolate the corresponding API. And I don't think this is a good thing for the user experience. IMO this could be a big entry barrier compare to now because they would need to understand which version of the library they would want to use.

Zaid-Ajaj commented 6 years ago

IMO this could be a big entry barrier compare to now because they would need to understand which version of the library they would want to use.

I agree, power pack is nice for most people because it covers a lot common application needs. However this is the distinction I want to make: applications can use Fable.PowerPack as it is now but libraries and library authors should only use the components they need. I suggest making Fable.PowerPack into a "meta package" that just includes multiple packages as dependencies like this:

Fable.PowerPack
 |
 | -- Fable.Fetch
 | -- Fable.Promise
 | -- Thoth.Json 

then the remoting client can be like this:

Fable.Remoting.Client
 | 
 | -- Fable.Fetch
 | -- Fable.Promise

This way users will still "just use" Fable.PowerPack without making anything obsolete, what do you think Maxime?

MangelMaxime commented 6 years ago

Ok so you want to create two new real bindings and then make Fable.PowerPack an helper on top of them.

I think it can be a good idea. If you want to open an issue describing a proposition so we can ask others opinion and review on the pros/cons :)

Zaid-Ajaj commented 6 years ago

Yeah Fable.PowerPack doesn't even need to include helper code, just include other libraries as dependencies, I will try to post the the suggestion on the main power pack repo soon

MangelMaxime commented 6 years ago

It will needs to include some helpers code for combining Fetch and Thoth.Json so people still have access to Fetch.asJson and so on.

nojaf commented 6 years ago

Thanks for your input. And so elephant in the room: can't you guys be friends again and converge both SimpleJson and Thoth into one serializing library? 😅

To me it just feels like you both solved the same problem, and I'm still not sure when to use Thot or when to use SimpleJson.

MangelMaxime commented 6 years ago

I think we kind of have the same situation as Fable-Arch vs Elmish at the time. Where we decided to use the library having more traction and that we considered more "powerful". At the time, the key difference has been the support for react by default compared to vdom.

Please note, that because I am the creator of Thoth.Json even if I try to be objective I am probably biased :p

I think both library covered more of less the same features: serializing and deserializing JSON. One feature that Thoth.Json have and that Fable.SimpleJson don't have (at the time of writing) is path tracking.

Error at: `$.user.firstname`
Expecting an object with path `user.firstname` but instead got:
{
    "user": {
        "name": "maxime",
        "age": 25
    }
}
Node `firstname` is unkown.

I do think, we should use Thoth.Json everywhere because the API is more clean and user friendly compared to using standard F# active pattern over strings.

This active pattern thing can also be seen as one of the force of Fable.SimpleJson because F# devs already know active pattern. But they already know functions in the case of Thoth.Json which are the basic blocks from F# perspective.

For comparaison:

type Person = 
    { Name: string
      Age: int }

// Fable.SimpleJson (copied from the README.md)

"{ \"name\":\"john\", \"age\":20 }"
|> SimpleJson.tryParse
|> function
    | Some (JObject dict) ->
        let value key = Map.tryFind key dict
        [value "name"; value "age"]
        |> List.choose id
        |> function
            | [JString name; JNumber age]  -> 
                Some { Name = name; Age = int age }
            | _ -> None
    | _ -> None

// Thoth.Json (decoding the same User json)

type Person = 
    { Name: string
      Age: int }

    static member Decoder =
        Decode.object (fun get ->
            { Name = get.Required.Field "name" Decode.string
              Age = get.Required.Field "age" Decode.int }
        )      

"{ \"name\":\"john\", \"age\":20 }"
|> Decode.fromString Person.Decoder 

If @Zaid-Ajaj , is ok for replacing Fable.JsonSimple with Thoth.Json I will be happy to answer his questions and add any missing features if needed :)

Zaid-Ajaj commented 6 years ago

can't you guys be friends again and converge both SimpleJson and Thoth into one serializing library?

@nojaf Of course we are still friends but we have different ways of solving problems, :smile:

Yes, both might be able to serialize and deserialize JSON into and from typed entities but my idea with SimpleJson is to work with JSON as a data structure: you can manipulate and transform it using standard F# functions and active patterns the same way you do with Newtonsoft.Json.Linq without defining intermediary types, for example, recently we had to work with this JSON from a web service:

{ 
  "12": { "value": 1, "key": "key1" },
  "34": { "value": 2, "key": "key2" }  
}

This is obviously bad way of communicating JSON so I used Newtonsoft.Json.Linq to normalize the structure into something easier to work with:

[
  { "id": 12,  "value": 1, "key": "key1" },
  { "id": 34,  "value": 2, "key": "key2" }  
]

I could do the same with SimpleJson as follows:

let normalize inputJson = 
    inputJson
    |> SimpleJson.tryParseNative
    |> Option.bind (function 
        | JObject dict -> Some dict
        | _ -> None)
    |> Option.map (fun dict -> 
        [ for (id, innerDict) in Map.toSeq dict do 
            let read xs = SimpleJson.readPath xs innerDict
            match read ["value"], read ["key"] with 
            | Some value, Some key -> 
                yield 
                  [ "id", JNumber (float id)
                    "value", value
                    "key", key  ]
                  |> Map.ofList
                  |> JObject

            | _ -> () ] 
        |> JArray 
    ) 

I used the same techniques when building the automatic converter and it was a joy to work with. I am not saying SimpleJson is "better" than Thoth.Json and I actually like the decoder/encoder API but I think they can have different use cases so I am not planning on deprecating SimpleJson, you can choose whichever API you like and the one suits your needs

MangelMaxime commented 6 years ago

@Zaid-Ajaj So in your example, you are actually modifying the JSON before sending over the network ? We resume it as doing JSON -> JSON manipulation directly.

Or are you running the normalize function in the client code just before "decoding" it into a real type ?

I am just trying to understand because this is perhaps here the difference we need to document :)

PS: I think there is room enough for several JSON library if we are able to explain clearly the difference and which one to use when :).

PS2: I have no experience at all with Fable.JsonSimple :)

Zaid-Ajaj commented 6 years ago

you are actually modifying the JSON before sending over the network

Yes, you can pre-process and post-process the JSON however you like as a data structure like list or Map. in this example you could enumerate the object properties and convert their values without knowing how many properties there are in that object which would not be possible using automatic deserializer

nojaf commented 6 years ago

@nojaf Of course we are still friends

Very glad to hear this 🤗.

So maybe to recap some remarks above:

Nhowka commented 6 years ago

Speaking of coupling, wouldn't it be helpful to have a decoder interface that SimpleJson and Thoth.Json could implement and have Fable.PowerPack depending on that interface?

Zaid-Ajaj commented 6 years ago

@Nhowka I don't think that kind of decoupling is needed for PowerPack, it should be enough to use Thoth.Json under the hood without having to make Thoth.Json swap-able with SimpleJson or others.

Zaid-Ajaj commented 6 years ago

Hello @nojaf this has been fixed in #86 and remoting client now uses XMLHttpRequest instead of fetch which makes things a lot simpler!

Can please you update to v4.5.0 to check if it still works as it should? the integration tests are running fine but I am not sure how it impacts an exisiting app ;)

nojaf commented 6 years ago

Hi @Zaid-Ajaj, I've updated and everything still works. I still have Fable.PowerPack (and thus Thoth.Json) in my project due to other references (Fable.Elmish f.ex).

But it's a start I guess, many thanks.