Replace Newtonsoft.Json with System.Text.Json #283
Conversation
| <ItemGroup> | ||
| <PackageReference Include="Newtonsoft.Json" Version="9.0.1" /> | ||
| <PackageReference Include="Newtonsoft.Json" Version="9.0.1" /> |
There was a problem hiding this comment.
Should we reference either one or another?
There was a problem hiding this comment.
And the another, already existing serializer must be included only if the target framework is lower than netstd2
There was a problem hiding this comment.
I don't see any problem in referencing Newtonsoft.Json in netstandard20 ...
I simply like the option to use System.Text.Json ...
maybe in the future with net50 things can be different and we can drop support for Newtonsoft.Json
| @@ -8,24 +11,45 @@ namespace JWT.Builder | |||
| public class JwtHeader | |||
| { | |||
| [JsonProperty("typ")] | |||
| #if SYSTEMTEXTJSON | |||
There was a problem hiding this comment.
Should it be one or another (conditionally)? I would define attribute alias in the top between #if #else #endif
There was a problem hiding this comment.
I really don't know .. freedom of choiche is a value ... using netstandard20 I can choose beetween the two types of serializers (like in the asp.net core framework) so why forcing to use System.Text.Json if even microsoft don't force us to do so?
There was a problem hiding this comment.
Otherwise "replacing" won't be replacing but rather adding one more dependency, what users of Json.Net won't need hence would rightfully question.
Ideally either serializer would need to go to its own satellite package so users would be free to choose between one or another or maybe something third.
My point is that having both attributes and serializers basically means referencing them all and piling their respective attributes on each of this property.
I can easily imagine Unity folks or someone on .NET 3.5 who's unable to use either and who use DataContractJsonSerializer instead coming here and asking to add a third one.
So instead I would switch from a 3rd party to the 1st party serializer (and hope most of the users would be fine with that).
There was a problem hiding this comment.
yes, I understand ... I leave this choice to you ... I will push another commit with some improvment ...
| var value = (JsonElement)odata[key]; | ||
| switch (value.ValueKind) | ||
| { | ||
| case JsonValueKind.Undefined: |
There was a problem hiding this comment.
Please group all cases that do nothing into a single block
| case JsonValueKind.Array: | ||
| break; | ||
| case JsonValueKind.String: | ||
| ndata.Add(key, value.GetString()); |
There was a problem hiding this comment.
And same here: multiple cases - one block of code
| // Implement using favorite JSON serializer | ||
| var data = JsonSerializer.Deserialize<T>(json); | ||
|
|
||
| if (data is Dictionary<string, object> odata) |
| } | ||
| } | ||
|
|
||
| return ndata is T obj ? obj : data; |
There was a problem hiding this comment.
Can you please explain this line?
There was a problem hiding this comment.
You cannot "return ndata" because ndata is an object of type Dictionary<string, object> not of type T. This expression cast ndata to the type T if possible. If it is not possible, default to return the object previuosly deserialized as is. Compiler know nothing about the relationship between T and Dictionary<string, object>, so this trick (or similar) is required.
|
What would you say if I propose to move the whole thing to a Ideally I'd Invision to move all Json.Net-dependent classes to a separate package as well. This idea was outline long time ago in #76. |
Now (only in netstandard20) you can use SystemTextJsonSerializer as a serializer.
Some limitations apply, resolves #218.