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

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

Closed
xperiandri opened this issue Apr 5, 2021 · 10 comments
Closed

Comments

@xperiandri
Copy link
Contributor

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

@Zaid-Ajaj
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

Can it be done as a generator option?

@Zaid-Ajaj
Copy link
Owner

Zaid-Ajaj commented Apr 6, 2021

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
Copy link
Contributor Author

xperiandri commented Apr 6, 2021

__typename can be handled using FSharp.SystemTextJson

@Zaid-Ajaj
Copy link
Owner

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
Copy link
Contributor Author

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

@Zaid-Ajaj
Copy link
Owner

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
Copy link
Owner

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

@Zaid-Ajaj
Copy link
Owner

Works like a charm as of v1.31 🚀

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

2 participants