Replacing Newtonsoft.Json with System.Text.Json

[queequac]

To reduce 3rd party dependencies and use netcore standard libraries instead, I'd favor the usage of System.Text.Json over Newtonsoft.Json. System.Text.Json is also known to be smaller and faster than Newtonsoft.

[queequac]

All tests are green again, could also verify lots of use cases with my real Xbox One. Only for the new InputTVRemoteChannel I had no chance to check this against a real device and had no test cases. I added a sample JSON message that looks like what the code expects to the test project and wrote a very quick and dirty test case. Maybe @mitchcapper can have a look at this and maybe provide a real JSON sample for such a message.

[mitchcapper]

I am happy to provide some samples and such for it.

My opinion wasn't asked but: I would be careful replacing Newtonsoft it is pretty ubiquitous and while I haven't used the new System.Text.Json for many hours, generally the paired down featuresets of MS"s json handlers can cause some issues.

We have some JSON handling but looking at what the python version implements there seems to be a whole lot more over the json channel.

[queequac]

Newtonsoft.Json is pretty ubiquious, because there was nothing meaningful for JSON part of the standard for a long time. I don't want to doubt that. :)

But the .NET Foundation created System.Text.Json to have some fast, stable and small JSON library in the framework itself instead of relying on a 3rd party library.

Personally I made pretty good experiences with System.Text.Json in production code. And while it is true that some features (compared to Newtonsoft) are missing, I do not see any fancy stuff in the channel messages that could not be handled with its API. Both are pretty close in their design anyway and this would be more a problem if you migrate from an existing code base which made use of some minions not available on the other side. But I noticed that even in these cases the gaps could be closed with very little custom code easily.

[tuxuser]

Maybe that decision should be postponed until all different JSON message bodies and according tests are implemented so we can see if there might be some shortcomings on System.Text.Json in comparison to Newtonsoft.Json.

I haven't used System.Text.Json so far, so it's hard for me to judge wether a change to the inbuilt module has needed feature set.

[queequac]

Not sure whether it is really useful to do all implementation work for currently missing message bodies with Newtonsoft first, doing the switch afterwards. That would be doubled effort. I'd prefer either to stick to Newtonsoft at all or to do the switch early doing the new stuff already with System.Text.Json.

From what I have seen so far, channel messages are usually quite small and simple and do not have complexity of large documents with fancy schemas.

Missing features are usually related to the serialization part, not deserialization of simple JSON snippets. (see also How to migrate from Newtonsoft.Json to System.Text.Json) Saying so, writing code for System.Text.Json will most likely not block anything (whole ASP.NET Core could replace Newtonsoft, I do assume they have do deal with more complex JSON handling) while migrating code afterwards might require major changes/refactoring if potentially done in a Newtonsoft-specific way with a C# construct that cannot be mapped one-to-one onto System.Text.Json.

Maybe we could look at some message bodies not implemented yet and judge based on them, e.g. if there is anything special?

[tuxuser]

Currently we are really not gaining any benefit from it because OpenXbox.SmartGlass depends on OpenXbox.XboxWebApi (https://github.com/OpenXbox/xbox-webapi-csharp), which in return depends on Newtonsoft.Json.

If this change happens, it needs to happen in the webapi project too imho.

[queequac]

OpenXbox.SmartGlass depends on OpenXbox.XboxWebApi

You are talking about SmartGlass.CLI, right? The SmartGlass library itself does not depend on XboxWebApi, and that's what I do depend on. ;) But I do agree, might be more reasonable if both could migrate together, since these projects are somehow related. Would consider to do so, but currently there are too many changes happening in the repo, so it's hard to catch up.

If it is okay for you, keep the issue open at the moment (maybe add a related one to WebAPI and link to this one) and I will take over once I find time and there is less commits here.

[mitchcapper]

Well XboxWebApi is needed unless you use another way to generate the auth hash/token (or only make anonymous calls)

[queequac]

Well XboxWebApi is needed unless you use another way to generate the auth hash/token (or only make anonymous calls)

I am indeed happy with anonymous calls, that's why I said I do depend on SmartGlass only. 😉