search

exceptionless / Exceptionless.Net

Exceptionless clients for the .NET platform
https://exceptionless.com
Other
554 stars 142 forks source link

Use System.Text.Json #251

Open benaadams opened 3 years ago

benaadams commented 3 years ago

An exploration; still needs some work

ejsmith commented 3 years ago

Nice! I'm definitely interested in making this change. We would need to make it a major version because it would definitely be breaking. We would also need to make it easy for users to still use JSON.NET to serializer their own models.

benaadams commented 3 years ago

Windows error seems to be around not being able to load the a dll for 462

benaadams commented 3 years ago

Bumping to net472 fixes it; but at that point is it just netstandard2.0?

ejsmith commented 3 years ago

https://www.nuget.org/packages/System.Text.Json

Looks like it targets 4.6.1. I’m definitely not against targeting just netstandard2.0 though. With the limited resources we have, I will prioritize ease of development over really old platforms that aren’t used by that many people.

benaadams commented 3 years ago

Looks like it targets 4.6.1.

Dropping to 4.6.1 looks to work. I assume targeting 4.6.2 means it picks up the netstandard2.0 project instead (as that's a later version than the 4.6.1); and then its in the grey area of netstd2.0 not quite working between 4.6.1-4.7.1 (for things like valuetuples etc)

benaadams commented 3 years ago

Hmm.. CI doesn't agree with my assessment

niemyjski commented 3 years ago

My biggest concern about this pr is taking a dependency on physical assemblies again. In the past a long long time ago... we ran into many runtime issue due to having no control over what version is loaded from say a VS Addon which might have multiple versions of JSON.NET (causing a crash reporting library to potentially crash the process). Also, there are some pretty crazy data types that could potentially get thrown at this. I hope Microsoft has solved both of these concerns. Another thought, for full framework we'd be forced to include any dependencies in our WIX installer at least for generator and this goes for our other customers as well (it's not the end of the world).

ejsmith commented 3 years ago

Yeah, we had a lot of diamond dependency issues with users apps when we had a direct dependency on JSON.NET. I think that is still somewhat of a concern, but it seems like things have gotten a lot better in .NET since then. Worst case, we could do the same source import for STJ. As far as crazy data types, ideally the things people attach to their events are pretty simple objects, but it has to be a requirement that they can serializer in a different way and honestly, I'd be fine with that other way being that they have to serialize themselves and add it as raw JSON.