-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: master
Are you sure you want to change the base?
[WIP] Migrating from Newtonsoft.Json to System.Text.Json #100
Conversation
56ee4d6
to
a9d69c3
Compare
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. |
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. |
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" |
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... |
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. |
@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 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. |
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. |
What kind of change does this PR introduce?
Migrating from
Newtonsoft.Json
toSystem.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.