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

Move library from Newtonsoft.Json to System.Text.Json #1995

Open
tipa opened this issue Nov 3, 2021 · 16 comments
Open

Move library from Newtonsoft.Json to System.Text.Json #1995

tipa opened this issue Nov 3, 2021 · 16 comments
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@tipa
Copy link

tipa commented Nov 3, 2021

Microsoft & many developers have been moving their apps & libraries from Newtsoft.Json to System.Text.Json recently (e.g. the Microsoft Graph SDK for .NET).
It would be very appreciated if this library could be moved as well. Not only is System.Text.Json faster and more efficient, it is also already used by many developers and libraries. Newtonsoft.Json isn't exactly the smallest .dll and increases the binaries of apps we are creating with the Google API Library for .NET.

This suggestion has been posted by @LindaLawton in #1464 (comment) before and I have read the discussion following on it but I was afraid that this doesn't get the visibility needed when not posted as an own feature request.

@tipa tipa added priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Nov 3, 2021
@jskeet
Copy link
Collaborator

jskeet commented Nov 3, 2021

The answer is basically the same though: it would be a breaking change, and one we basically wouldn't take unless we moved to a new major version, which is rather unlikely.

We hope to eventually removed the dependency on Google.Apis.Auth and indirectly Newtonsoft.Json for the Google.Cloud.* libraries, but even that's a long-term goal rather than anything imminent.

@LindaLawton
Copy link
Collaborator

LindaLawton commented Nov 4, 2021

@jskeet I understand and respect your dedication for the new Google.Cloud.* library.

However the thing is that the Google.Apis.* library has been around a lot longer. I have personally been using it at least since 2011. This library is not going away just because there is the new shinny Google.Cloud.* library which does not support most of the discovery services apis that this library supports. We have users of this library which deserve your support and dedication as well.

As long as Microsoft continues to release new versions of .Net this library needs to and must grow with that and not become stagnant.

I would like to add if I remember correctly. There was a time when you removed support for .net 3.5 without telling the community at all. That was a huge breaking change for me personally one day it worked the next it didn't. So I don't think you should be that concerned with breaking changes. I just think that they should be voiced to the community first this time.

@tipa tipa changed the title Move library to from Newtsoft.Json to System.Text.Json Move library from Newtsoft.Json to System.Text.Json Nov 4, 2021
@jskeet
Copy link
Collaborator

jskeet commented Nov 4, 2021

I don't think Json.NET is going anywhere though. Making a breaking change and releasing a new major version feels like it would be significantly disruptive. I don't think it's worth that just to remove a dependency, when we don't expect to make other significant changes.

We'll keep supporting this library and releasing new API versions, but that doesn't mean we're going to put in the large amount of effort and cause the large amount of disruption that this would cause, when the existing situation may not be ideal but works fine.

I would like to add if I remember correctly. There was a time when you removed support for .net 3.5 without telling the community at all. That was a huge breaking change for me personally one day it worked the next it didn't. So I don't think you should be that concerned with breaking changes. I just think that they should be voiced to the community first this time.

I don't remember the details of that exactly (and if we really didn't make any announcement, I regret that.). But I suspect that it only happened after .NET 3.5 itself had stopped being supported by Microsoft for a significant length of time. That's very different from making a breaking change to supported platforms. I am concerned with breaking changes, and I don't think you'll convince me that I shouldn't be.

@jskeet jskeet changed the title Move library from Newtsoft.Json to System.Text.Json Move library from Newtonsoft.Json to System.Text.Json Nov 4, 2021
@LindaLawton
Copy link
Collaborator

I suspect that it only happened after .NET 3.5 itself had stopped being supported by Microsoft for a significant length of time.

Well then If we follow along that train of thought and we know already that .NET Framework 4.5.2, 4.6, 4.6.1 will reach End of Support on April 26, 2022

Can we expect it to be addressed at the time we remove support for 4.5?

If so do we have a definition for "significant length of time"?

@jskeet
Copy link
Collaborator

jskeet commented Nov 4, 2021

I think we should address the detailed supported framework timelines in a separate issue, keeping this one to the Newtonsoft.Json dependency. However, I wouldn't expect to create a new major version just to remove support for frameworks that are already end-of-life by Microsoft. So if we decide to stop supporting .NET 4.5, I don't think we'd take a major bump for that. We would definitely try to make sure it's communicated well though. (I think it's more likely that we'll drop support for netstandard1.3 to be honest - but again, we'd communicate that clearly.)

In general (across GAX and Cloud libraries) we take a major version bump as an opportunity to reassess our supported platforms, but I don't think we should penalise developers who are already making sure they stay up-to-date with supported platforms by making breaking changes unless we have a really good reason to, and again I don't think the desire to drop a working dependency is a sufficient reason to create the breaking change.

@LindaLawton
Copy link
Collaborator

LindaLawton commented Nov 4, 2021

Not only is System.Text.Json faster and more efficient, it is also already used by many developers and libraries. Newtonsoft.Json isn't exactly the smallest .dll and increases the binaries of apps we are creating with the Google API Library for .NET.

Already working is not a question but is it the best solution? Efficiency and size are very important IMO.

@jskeet
Copy link
Collaborator

jskeet commented Nov 4, 2021

No solution is going to be perfect for everyone - but given that these libraries are in maintenance mode, I don't regard this as a critical issue.
I guess in a perfect world we'd have multiple major versions so that those who wish to stay on 1.x can do so without taking breaking changes (but still getting new API versions) and a parallel 2.x which has taken all the breaking changes we might want to. But we just don't have the resources to do that, I'm afraid.

@LindaLawton
Copy link
Collaborator

As always we appreciate everything you and the team do. Maintaining an OS project is a thankless job. Let me take the time to Thank you.

Please keep us updated on this.

@guillaume86
Copy link

Meanwhile, would it be acceptable to just add the [JsonPropertyName] attributes for a NET_50_OR_GREATER target?

I don't think that would be considered a breaking change but would solve my case (serialization to a jsonb column with EF Core, I do not have control on the serializer that is used).

@jskeet
Copy link
Collaborator

jskeet commented Nov 18, 2021

@guillaume86: Please could you file a new issue with the details of what you're trying to achieve, and why it needs new attributes to be applied? It sounds like it's a somewhat different use case than "Please migrate off Newtonsoft.Json" in this case. (Can your serialization not just use the default property names?) There's some potential there, although we wouldn't target .NET 5 as it's not an LTS - we'd make it .NET 6.0+, if we do it at all. To set expectations, I doubt that we'll be able to put much time into even considering this until Q1 of 2022.

@guillaumejay
Copy link

I think it's a real issue, because #1983 is alive on AzureDevops, and breaks CI/CD.

@jskeet
Copy link
Collaborator

jskeet commented Dec 10, 2021

@guillaumejay: As I noted on that issue, it's a problem with dotnet publish/dotnet deploy, and there's a fairly simple workaround.

I'm not going to start making breaking changes because some tooling doesn't know how to handle dependencies properly. That's just a losing proposition - it doesn't fix the problem at its cause, it just temporarily alleviates it. Then you end up with some other dependency which expects versioning to work in the normal way, and you're back to square one.

@guillaumejay
Copy link

In fact, I was thinking that the workaround was about --output flag, which is not possible on AzureDevOps.
The workaround about using filtered solutions is working. It's not great, because it means I have no tests in my CI.
But still, it works now.

Thanks !

@tipa
Copy link
Author

tipa commented May 6, 2022

.NET Framework 4.6.1 (among others) has reached end-of-support now so that's one obstacle less :)
Would love to see System.Text.Json to be used at some point in the future

@jamiehankins
Copy link

I was so happy to be rid of Newtonsoft nonsense. The System.Text.Json stuff works quite well.

Then, we had to implement the weird Rube Goldbergesque acrobatics that are required to get through an Identity Aware Proxy, which involves using this library. Imagine my surprise to see that I've been reinfected by transitive dependency.

I see the argument above that changing JSON serialization libraries is a breaking change. How so? Admittedly, I'm very new to your library, but is it really designed in such a way that there is an external effect caused by changing something that should be internally opaque?

Places where you serialize JSON should take an object and output a string (or UTF8 byte collection). Places where you deserialize should take a string (or UTF8 byte collection) and output an object. Newtonsoft does this. System.Text.Json does this. Where's the break?

@jskeet
Copy link
Collaborator

jskeet commented Apr 27, 2023

@jamiehankins: Here are three different ways in which this would be a breaking change:

  • It would break the build of anyone who uses Newtonsoft.Json in their own code, but are relying on the transitive dependency. If this were the only form of break, I think we'd at least strongly consider doing it anyway, because it's very easy to fix by adding a direct dependency.
  • It would break anyone using Newtonsoft.Json to serialize or deserialize either API-specific types or other types such as JsonCredentialParameters, which are decorated with Newtonsoft.Json attributes to control JSON naming etc. We could easily add the equivalent System.Text.Json attributes instead, but it would still break anyone using Newtonsoft.Json.
  • We expose public properties using Newtonsoft.Json types, most notably NewtonsoftJsonSerializer. Removing the Newtonsoft.Json dependency would mean those classes would have to be removed as well, which is clearly a breaking change. It's possible that with type forwarding we could move them to a different project, but it would at least be very fiddly.

Just this week I was investigating whether it would be possible to drop the dependency from Google.Apis.Auth to Google.Apis/Google.Apis.Core, partly in order to remove the transitive dependency on Newtonsoft.Json - the second bullet above is the most concerning one for that. It's unfortunate that as far as I'm aware, there's no "serializer-neutral" set of attributes - it would be great if there were a package defining those, with both Newtonsoft.Json and System.Text.Json observing them. But without that, I don't think we'd be able to remove Newtonsoft.Json from these packages without a new major version containing significant breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

6 participants