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

Use System.Text.Json #251

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Use System.Text.Json #251

wants to merge 4 commits into from

Conversation

benaadams
Copy link
Contributor

An exploration; still needs some work

@ejsmith
Copy link
Member

ejsmith commented Mar 21, 2021

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
Copy link
Contributor Author

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

@benaadams
Copy link
Contributor Author

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

@ejsmith
Copy link
Member

ejsmith commented Mar 30, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Hmm.. CI doesn't agree with my assessment

@niemyjski
Copy link
Member

niemyjski commented Mar 31, 2021

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
Copy link
Member

ejsmith commented Mar 31, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants