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

Remove JsonValue.fs from client and use System.Text.Json #328

Open
xperiandri opened this issue Apr 3, 2021 · 10 comments
Open

Remove JsonValue.fs from client and use System.Text.Json #328

xperiandri opened this issue Apr 3, 2021 · 10 comments

Comments

@xperiandri
Copy link
Collaborator

Description

The JsonValue.fs deserializer deserializes 0 into int while it must deserialize it into the type requested by the schema. Which is float in my case.
We can simply use System.Text.Json package version 5 as it is done in https://github.com/fsprojects/SwaggerProvider/tree/net5 with https://github.com/Tarmil/FSharp.SystemTextJson/

Repro steps

  1. Define Float field in GraphQL and return 0.0 from it
  2. Use System.Text.Json as a serializer
  3. You will get int option

Expected behavior

Value is deserialized into the type defined by the schema

Actual behavior

Value is deserialized into the type chosen by JsonValue.fs logic

Related information

1.0.7

@njlr
Copy link
Contributor

njlr commented Oct 9, 2022

There are a few things that the homegrown JSON (and the more widely used Thoth.Json.Net) offers:

  1. Fine-grained control over encoding and decoding without using reflection, annotations or writing "DTO" types:
type Money = 
  {
    Dollars : int
    Cents : int
  }

// JSON representation is:
//     "123.45"

let decode = 
  decoder {
    let! (x : string) = Decode.string

      match x.Split([| '.' |]) with
      | [| dollars; cents |] ->
        let! dollars = 
          match Int32.TryParse(dollars) with
          | true, i -> Decode.succeed i
          | _ -> Decode.fail "Invalid dollars"

        let! cents = 
          match Int32.TryParse(cents) with
          | true, i -> Decode.succeed i
          | _ -> Decode.fail "Invalid cents"

        return { Dollars = dollars; Cents = cents }
      | _ ->
        return! Decode.fail "Expected <dollars>.<cents>"
  }
  1. Ability to visit a JSON structure in a type-safe manner using match
let json = Decode.unsafeFromString "{ \"foo\": 123 }"

match json with
| JsonValue.Object m ->
   match Map.tryFind "foo" m with
   | Some (JsonValue.Integer x) -> x
   | _ -> 0
| _ -> 0
  1. Ability to compare JSON values as values:
let a = Encode.object [ "foo", Encode.int 123 ]
let b = Encode.object [ "foo", Encode.int 123 ]

Assert(a = b)
  1. Support for Fable - we should at some point bring back support for Fable in the client type-provider

Does System.Text.Json offer these?

If not, perhaps they could be built on-top using active patterns?

@xperiandri
Copy link
Collaborator Author

@gusty could you share your opinion?

@gusty
Copy link
Member

gusty commented Oct 20, 2022

I agree with @njlr but would add that Fleece offer those same advantages and more (plus it has better performance) only Fable compatibility is still pending at the time of writing this, as no-one added a Fable compatible encoding implementation so far.

@njlr
Copy link
Contributor

njlr commented Oct 24, 2022

I agree with @njlr but would add that Fleece offer those same advantages and more (plus it has better performance) only Fable compatibility is still pending at the time of writing this, as no-one added a Fable compatible encoding implementation so far.

I had a glance at Fleece - it looks great!

However, would we need to have an FSharp.Data.GraphQL package for each Fleece back-end?

  • FSharp.Data.GraphQL.NewtonsoftJson
  • FSharp.Data.GraphQL.SystemTextJson
  • ...

@xperiandri
Copy link
Collaborator Author

xperiandri commented Oct 24, 2022

I expect to use System.Text.Json only

@wallymathieu
Copy link
Member

Why specifically System.Text.Json ?

@xperiandri
Copy link
Collaborator Author

Do you have any scenario where you need something else?

@wallymathieu
Copy link
Member

I mean, the client shouldn't care about specific Json implementations? I don't know how many times I've had to inject some other Json library or configuration in order to get specific use cases to work. The best would be for it to be pluggable with a default that uses a simple enough interface.

@xperiandri
Copy link
Collaborator Author

I don't have the capacity for that. I sponsor part of this library development from my sallery. So if someone wants to present a solution I ready to discuss. But myself I only interested in System.Text.Json

@wallymathieu
Copy link
Member

I don't have the capacity for that. I sponsor part of this library development from my sallery. So if someone wants to present a solution I ready to discuss. But myself I only interested in System.Text.Json

For sure, I know the feeling. There is a limited number of hours I can use to support other open source projects.

Since the library uses explicit knowledge that JsonValue has patterns for String,Null,Record,Array et.c., you could use the same strategy as @gusty has done in Fleece by defining active pattern helpers. I looked at the API surface that is used in the project #400 .

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

4 participants