Giraffe icon indicating copy to clipboard operation
Giraffe copied to clipboard

Published 20 hours ago • verified publisher icon giraffe-fsharp

Make System.Text.Json the default JSON serializer

Open esbenbjerre opened this issue 1 year ago • 4 comments

Description

This PR removes the Newtonsoft JSON serializer and promotes System.Text.Json as the default. I think System.Text.Json is the better choice these days. Among other things the default serializer

  • Uses camel-case
  • Serializes Option as value or null
  • Supports ignoring Option or null values using the [<JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)>] attribute

Any other specific behaviour should IMO be implemented by users, using attributes (e.g., [<JsonPropertyName("someName")>], [<JsonConverter(typeof<SomeCustomJsonConverter>)>]), as there's no sensible defaults for most user-defined types.

How to test

Run tests and try to serialize/deserialize a JSON payload in a HttpHandler.

Related issues

https://github.com/giraffe-fsharp/Giraffe/issues/481 https://github.com/giraffe-fsharp/Giraffe/issues/560 https://github.com/giraffe-fsharp/Giraffe/issues/451

esbenbjerre avatar Oct 14 '23 19:10 esbenbjerre

@dustinmoris

esbenbjerre avatar Oct 15 '23 08:10 esbenbjerre

Why are you removing Newtonsoft entirely? I can see maybe swapping the default but not outright removal.

TheAngryByrd avatar Oct 15 '23 12:10 TheAngryByrd

It was my impression from discussion in #451 that it would be preferable to have it in a separate NuGet package and I agree with that sentiment. No reason to include the dependency by default if it's not going to be used. I also think System.Text.Json have become much better for F# with .NET 6/7 updates.

esbenbjerre avatar Oct 15 '23 12:10 esbenbjerre

I also think this switch would get us closer to finally having a tryBindJson function. I'll start working on that when/if this PR is approved.

esbenbjerre avatar Oct 19 '23 08:10 esbenbjerre

I'll re-review this PR during this week. Thanks for the updates @esbenbjerre!

64J0 avatar May 14 '24 20:05 64J0

Thanks for the heads-up @TheAngryByrd! Since this PR is already open for a while, and we have many approvals, I'm going to merge it right now. Thanks for the contribution @esbenbjerre, great job!

64J0 avatar May 18 '24 00:05 64J0

JFYI – That turns out to be a massive breaking change to one of my applications which runs on Giraffe since 4 years. Not blaming anyone, but having some change log which makes things like that more explicit than the release page on GH would be nice ✌️

aspnetde avatar Jul 23 '24 07:07 aspnetde

Hi @aspnetde, sorry for this breaking change. @abelbraaksma gave some suggestions at this issue on how to improve for future updates.

but having some change log which makes things like that more explicit than the release page on GH would be nice

Can you please expand this comment? What would you like to have found regarding this change?

64J0 avatar Jul 23 '24 11:07 64J0