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

[WIP] Migrating from Newtonsoft.Json to System.Text.Json #100

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kav
Copy link

@kav kav commented Sep 3, 2024

What kind of change does this PR introduce?

Migrating from Newtonsoft.Json to System.Text.Json per supabase-community/supabase-csharp#96.

This PR is WIP for visibility and early discussion. 6 tests are failing and untested behavior may still be broken. I've tried to limit changes as much as possible but this does seem to touch... a lot.

@kav kav force-pushed the feature/system-text-json-issue-96 branch from 56ee4d6 to a9d69c3 Compare September 3, 2024 04:37
@kav
Copy link
Author

kav commented Sep 12, 2024

Just checking if there is in fact appetite for this or any course corrections or guidance before I dive back in @acupofjose. Happy to take any maintainer edits or advice as well as I recognize this a heck of first PR.

@wiverson
Copy link
Collaborator

IIRC I think there were issues switching with Unity and possibly other tools/frameworks? Plus I think there were some packages that used NS specific processing, so it's not just this package the whole thing needs to be done.

@kav
Copy link
Author

kav commented Sep 12, 2024

I'm aware it's more than just this package; but you've got to start somewhere if you intend to start at all. I know there are realtime changes needed and I did see Unity as well. Not sure on the NS changes needed, can you say more there? Or is that a polite way of saying "don't bother"?

I can work around my parser collisions with pg_graphql/GraphQL.NET or some rather gross workarounds so I'm not wedded to addressing this. That said if there isn't an appetite to ever complete this work it's worth closing supabase-community/supabase-csharp#96 as "won't fix"

@wiverson
Copy link
Collaborator

Oh, I think this is one of those things that will evolve, so won't fix vs eventually fix are two different things.

I believe that there are portions of the existing code that rely on NewtonSoft (NS) specific attributes. I didn't work on any of that so @acupofjose would have to comment.

I think the PR to switch would involve building and testing the whole stack, including all tests and also validating that it all works with Unity as well. 🤷‍♂️

I'm hoping a future version of Unity switches to a more modern/less hacked up .NET in the future but IL2Cpp and the Unity runtime are a bit of a mess. I've been mostly working on Godot lately which uses a much more modern .NET, but AFAIK Unity runs about 3x usage of Godot.

It's all changing/in flux.

Plus of course all the different other versions of .NET...

@kav
Copy link
Author

kav commented Sep 12, 2024

Yeah I hear you on building and testing everything and that it's an evolution, I'm just trying to start the evolution. Given the original request was a year ago that felt reasonable. Sounds like now is maybe not the right time? Or a core contributor needs to start the effort given the scope and likely functional shift? Or core contributors don't have the bandwidth to assist? Which again is fine by me.

If you are saying PRs in this area are not welcome would you please say something a bit more direct? I'm struggling a bit parsing "well there is a lot to do here" as that has been and will remain true for a structual change that hits multiple downstream libraries. This WIP PR was intended to represent the first step not be a final success here.

@acupofjose
Copy link
Collaborator

@kav - really appreciate your work on this - and my apologies for not getting back to you sooner. I do agree that ideally we'd be using the System.Text.Json libraries for all of this as it'd almost certainly give a substantial speed boost to operations within the library. However, because of the scope of it, it probably needs to be tag-teamed with a core maintainer.

Unfortunately, I'm taking a step away from maintaining the Supabase libs, so unless @wiverson wants to spearhead this, I think now is just bad timing.

@kav
Copy link
Author

kav commented Sep 14, 2024

Thanks for the note @acupofjose. Sounds like you are right. I've POCed Strawberry Shake with pg_graphql for my particular use case and it works great. One other (somewhat more complex) option to consider is adding support for both libraries as Graphql.net does by specifying an option on init. The big advantage there would be some of the Unity cases and whatnot could likely be left NS only. That would likely warrant some simplification (eliminate the double serialization) and plugablity on the serialization path as an alternate first step to the above.

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

Successfully merging this pull request may close these issues.

3 participants