Zaid-Ajaj / Snowflaqe

A dotnet CLI to generate type-safe GraphQL clients for F# and Fable with automatic deserialization, static query verification and type checking
MIT License
154 stars 25 forks source link

Use System.Text.Json and FSharp.SystemTextJson instead of Newtonsoft #36

Closed xperiandri closed 2 years ago

xperiandri commented 3 years ago

https://github.com/Tarmil/FSharp.SystemTextJson/

Zaid-Ajaj commented 3 years ago

Unfortunately, this is a bit of a really hard requirement. Snowflaqe uses Fable.Rempting.Json (which uses Newtonsoft.Json underneath) to handle the JSON serialization and deserialization. The thing about Fable.Remoting.Json is that it knows how to handle GraphQL unions and interfaces by default using a __typename discriminator (which is a required field to select in your GraphQL query). See implementation here.

Given that the library is very well tested, is plenty fast and that changing the JSON handling is a huge undertaking, I don't see an immediate reason to use STJ yet.

That said, it is possible to generate the custom handling of GraphQL unions and interfaces into the generated project and make FSharp.SystemTextJson use but like I said, the incorporated solution is already good enough.

Zaid-Ajaj commented 3 years ago

Oh I forgot to mention that Fable.Remoting.Json also handles [<StringEnum>] attribute for GraphQL enums that allows to normalize their names

[<StringEnum>]
type ItemType = 
    | [<CompiledName "PULL_REQUEST">] PullRequest
    | [<CompiledName "ISSUE">] Issue
xperiandri commented 3 years ago

Can it be done as a generator option?

Zaid-Ajaj commented 3 years ago

Yes it can be but as mentioned above, it will require extra logic to handle enums that use [<StringEnum>] and somehow understand GraphQL unions / interfaces when it comes to the __typename discriminator. Otherwise it won't work. Ideally, I don't want to ship an option to enable a faster JSON library that isn't guaranteed to work for all GraphQL queries. If you plan on implementing such huge undertaking, please let us discuss the design and implementation beforehand. This is because such feature affects the code generation of other options such as #29 and #31 which are higher up on the priority issues

xperiandri commented 3 years ago

__typename can be handled using FSharp.SystemTextJson

Zaid-Ajaj commented 3 years ago

Let's discuss an implementation first. I think it would be better to start off with building a better testing infrastructure for this project where we generate, run and test the (generated) code against a live GraphQL server which returns all these different kinds of results.

xperiandri commented 3 years ago

Could you point me to the core places where Newtonsoft is used?

Zaid-Ajaj commented 3 years ago

First of all, a package reference is added here which you now change to use FSharp.SystemTextJson instead. Then the only places where it used is inside the generated client and its members such as this one

The code that generates the client is a bit ugly and stringy because it wasn't made with the idea that there will be many different variants. I think we will need a better API instead of using string templates. Ideally the members are generated via the F# AST but that could make matter more complicated. I am all ears if you have ideas for improving the API 💯

Zaid-Ajaj commented 3 years ago

Adding System.Text.Json now works since #48 was merged in. Only adding FSharp.SystemTextJson is left for resolving this issue

Zaid-Ajaj commented 2 years ago

Works like a charm as of v1.31 🚀