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

Replace Newtonsoft.Json with System.Text.Json with proper source generation #1322

Open
cyrmax opened this issue Dec 11, 2023 · 10 comments
Open

Comments

@cyrmax
Copy link

cyrmax commented Dec 11, 2023

I would like to request/propose a feature

I suggest replacing NewtonSoft.Json with System.Text.Json with proper source generation used.
The reason is that with newtonsoft we cannot use Aot and/or trimming which then breaks the entire Json package.
With System.Text.Json we can see the similar issue with trimming and Aot. But there we can use source generation aproach and avoid any errors when trimming the executable or building native code.
Here is the documentation for this source generation feature in detail and System.Text.Json docs can be googled easily.
https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation?pivots=dotnet-8-0

If anybody has any arguments against my feature request I would be glad to discuss them.
Also I will try to implement the thing I want by myself and if I achieve what I want without any errors I of course will make a pull request to address this issue.

Thanks in advance.

@karb0f0s
Copy link
Member

karb0f0s commented Dec 12, 2023

@cyrmax wow, what a great idea! How would you approach this transformation with the existing codebase?

@tuscen
Copy link
Member

tuscen commented Dec 12, 2023

Will this work for netstandard2.0 targets? Old runtimes that support it is still widely used and we can't stop supporting it yet.

@cyrmax
Copy link
Author

cyrmax commented Dec 12, 2023

@tuscen
I found the following information regarding your question.
TLDR: yes.
But here is a quote and a link.

The System.Text.Json library is included in the runtime for .NET Core 3.1 and later versions. For other target frameworks, install > the System.Text.Json NuGet package. The package supports:
• .NET Standard 2.0 and later versions
• .NET Framework 4.7.2 and later versions
• .NET Core 2.0, 2.1, and 2.2

And here is the link:
https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-8-0

@tuscen
Copy link
Member

tuscen commented Dec 12, 2023

@cyrmax Thanks, we'll probably try to migrate to source generated converters then

@cyrmax
Copy link
Author

cyrmax commented Dec 12, 2023

@karb0f0s
If you mean that it is impossible or too difficult then please explain why.
I didn't looked at the code yet but I think at least it is possible and reasonable.

@tuscen
Copy link
Member

tuscen commented Dec 12, 2023

@cyrmax By the way, you really don't want this library to be trimmed since it's an API client. What will it do in case your bot receives an object that should be deserialized to a type that was trimmed during compilation?

@tuscen
Copy link
Member

tuscen commented Dec 14, 2023

@cyrmax Do you have experience with STJ source generators? I’ve tried to use it in this library, but for some reason it doesn’t generate converters. But in a separate test project I don’t have any problems.

@cyrmax
Copy link
Author

cyrmax commented Dec 15, 2023

@tuscen I will try to investigate this on weekend. In my projects this feature works just fine too. I will see.

@tuscen
Copy link
Member

tuscen commented Dec 18, 2023

I've managed to generate json converters, but I struggle to understand how to make it work in .NET 6 in such a way that one could augment existing JsonSerializerOptions instead of completely rewriting existing ones. This is a deal breaker since we can't impose our own instance of JsonSerializerOptions on clients using webhooks since they might have their own conventions in their ASP.NET Core apps. It seems .NET 8 have a solution for this using JsonTypeInfoResolver.Combine which .NET 6 lacks. If you have ideas on how to workaround please tell me. Otherwise I don't think we will be able to use STJ until .NET 6 is EOL so we could target the library to .NET 8 instead of .NET 6.

@tarasverq
Copy link

I've managed to generate json converters, but I struggle to understand how to make it work in .NET 6 in such a way that one could augment existing JsonSerializerOptions instead of completely rewriting existing ones. This is a deal breaker since we can't impose our own instance of JsonSerializerOptions on clients using webhooks since they might have their own conventions in their ASP.NET Core apps. It seems .NET 8 have a solution for this using JsonTypeInfoResolver.Combine which .NET 6 lacks. If you have ideas on how to workaround please tell me. Otherwise I don't think we will be able to use STJ until .NET 6 is EOL so we could target the library to .NET 8 instead of .NET 6.

https://www.planetgeek.ch/2022/10/15/using-system-text-json-alongside-newtonsoft-json-net/
In this article author shows how to use System.Text.Json alongside Newtonsoft.
And also he shows how it's possible to change serializer options for endpoints marked with special attribute (file SystemTextJsonBodyModelBinder.cs lines 10-11)

Maybe it would be useful for further research.

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

4 participants