-
Notifications
You must be signed in to change notification settings - Fork 108
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
Any plans to switch to System.Text.Json? #189
Comments
👋 Hi there! Thanks for your contribution to the project by posting your first issue! |
@evgeni-nabokov Thanks for the comment. No plans right now in a way of ‘if it isn’t broke don’t fix it’ mindset :-). I think it would take some benchmark compared for our scenario here to Eakins if it is worth the effort right now. |
I thought I'd give this a go as an intellectual exercise - see how painful it would be. https://github.com/stoiveyp/alexa-skills-dotnet/tree/NoNewtonsoft Like-for-like changes aside, the really big issue seems to be that the JsonConverter attribute in System.Text.Json can't be applied to interfaces - which is something we really rely on for the polymorphism. Even with custom converters applied to the serializer options it deserializes them properly, but doesn't serialize an interface properly (won't serialize the properties that are only in that specific inmplementation - only the interface definition itself) Gonna keep at it - I enjoy the challenge - but I might try looking at the System.Text.Json source and see if I'm missing something. |
Nice thanks for being willing to experiment! As I mentioned, I'm not opposed to it but would also want to think downstream (your plugins) and would they be forced to change? On the note on interfaces, wonder if @terrajobst knows if this is planned or perhaps a way of achieving it. |
Absolutely right that none of the extensions would be supported, even if the conversion were flawless - the core library and each extension would need to be double tagged so we could continue I've not looked into how that would work but I'm guessing extensive use of compiler switches to ensure only the right attributes were applied to every class and property? That's a lot of maintenance (or possible a single subclassed attribute that isolates and hide the compiler switch maybe? _adds to list of things to look at) It's also the upstream issue; although I appreciate it's not exclusive - our primary use case is AWS Lambda and that's not available in a .NET 3.1 runtime flavour yet without custom layers. Also the LambdaSerializer supported by Amazon is one based on Newtonsoft - so we'd have to customise that too. Hugely interesting to investigate though, genuinely fun :) |
So I just hit this... updating from netcoreapp2.2 to 3.1, by default you get System.Text.Json wired up and then you'll end up getting a runtime exception modelbinding SkillRequest, etc. You can obviously fix with the AddNewtonsoft extension... I went down the route of conditionally including the correct attributes, but started to lose interest with 500 remaining errors... :) |
So I don't forget, this is the issue that would allow our "converter on interface" approach to so much of the ecosystem to be supported in System.Text.Json, currently against .NET 5.0 (although there is an interesting workaround I'm going to have a play with this week in my branch) |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I guess this requires total rewrite (different attributes etc.). What are your thoughts?
The text was updated successfully, but these errors were encountered: