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

Split out serialization format from main library #981

Closed
clairernovotny opened this issue Oct 6, 2020 · 8 comments · Fixed by #1005
Closed

Split out serialization format from main library #981

clairernovotny opened this issue Oct 6, 2020 · 8 comments · Fixed by #1005
Assignees

Comments

@clairernovotny
Copy link
Member

Now that we have abstracted the serialization formatter, we should split out the implementation from the main library. That is, we should have a Newtonsoft.Json implementation and a System.Text.Json implementation that can be opted-in to.

This will need some design. Things to consider:

  1. There should be a default. Perhaps System.Text.Json since it is a framework reference in .NET Core and doesn't need an explicit dependency there.
  2. This will be a breaking change no matter what we do. Flipping the default, moving code to another package, etc.
  3. Naming of packages -- what do people install? Refit.Newtonsoft.Json, Refit.Xml, Refit.System.Text.Json? What is in Refit, do people install Refit directly anymore?

See also: #980

@clairernovotny
Copy link
Member Author

Tagging @bennor @anaisbetts @shiftkey for discussion.

@anaisbetts
Copy link
Member

anaisbetts commented Oct 6, 2020

The most important thing is that, if you're installing Refit into a new app, you literally Do Not Think about this at all - it all Just Works.

To that end, I think that we should not consider these two to be on equal footing, I think that we should treat Newtonsoft.Json as a Legacy compatibility package and require it to be opt-in (i.e. people have to install a separate package, and call some initialization method etc etc) - let people configure the old one if they need it, but otherwise they just get the new one, live happy lives with the headache they Don't Know they Don't Have, impress their boss with the incredible productivity of Refit, then retire having lived a stress-free developer life 😅

@clairernovotny
Copy link
Member Author

@anaisbetts I'm down with that plan. The biggest thing is that Json.Net is more flexible than the system one and people may be relying on it. But as a major version/breaking change, they can opt-in to the legacy behavior. Any suggestions on what to call that legacy package?

@anaisbetts
Copy link
Member

I think that Refit.Newtonsoft.Json is good, so most people install Refit and the Newtonsoft folx install Refit and Refit.Newtonsoft.Json

@bennor
Copy link
Contributor

bennor commented Oct 7, 2020

I don't disagree with any of the above. It makes sense for the built-in to be the default, and I wouldn't expect it to make much difference for the majority of users.

@antoinebj
Copy link

antoinebj commented Nov 10, 2020

I don't disagree with any of the above. It makes sense for the built-in to be the default, and I wouldn't expect it to make much difference for the majority of users.

I know it might be just semantics, but is anything really "built-in" anymore? System.Text.Json seems to just be another NuGet package. If an application doesn't depend on it, it won't be there, can we really call it "built-in"?

Now although I don't have any strong opinion on the matter of how to organize packages, I do prefer to make sure that all options be on the table. So for the sake of completeness, I'll throw this out there:
You could call the opinionated package Refit, but put the agnostic part in a Refit.Core or Refit.Engine package that would be used by the Refit.Newtonsoft.Json package. Then if anyone later comes up with some other popular content serializer (maybe not even JSON), or someone creates a System.Text.Json.Abstractions package or some popular equivalent, Refit.Whatever packages can be created that only depend on Refit.Core and don't bring along redundant System.Text.Json implementations.

@clairernovotny clairernovotny self-assigned this Nov 13, 2020
@clairernovotny
Copy link
Member Author

clairernovotny commented Nov 13, 2020

I know it might be just semantics, but is anything really "built-in" anymore? System.Text.Json seems to just be another NuGet package. If an application doesn't depend on it, it won't be there, can we really call it "built-in"?

Yes, it is. System.Text.Json quite literally is in the framework and is built-in. I'd prefer not adding a third package here. It's only another NuGet package for older targets.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants