canton7 / RestEase

Easy-to-use typesafe REST API client library for .NET Standard 1.1 and .NET Framework 4.5 and higher, which is simple and customisable. Inspired by Refit
MIT License
1.07k stars 107 forks source link

Looking forwards to RestEase 2 #150

Open canton7 opened 4 years ago

canton7 commented 4 years ago

At some point, I'd like to move to RestEase 2. There are two main things want to achieve:

  1. Make use of source generators
  2. Make Json.NET optional

This issue is a brain dump, and a place to record my thoughts as they evolve.

Source Generators

Source generators are coming. They will essentially let NuGet packages hook into the build system, and insert their own code. This is a perfect fit for something like RestEase, where we can generate the implementation of a user's interface at compile-time instead of at runtime.

However, I don't want to remove support for the current IL emission approach.

As I understand it, the options available are:

  1. Let the user define partial classes with partial methods instead of interfaces, and generate the other part of the partial class.
  2. Keep the current approach of letting the user define an interface, and generate an implementation for that.

Partial classes have some appeal: they would let users write:

partial class MyApi
{
    [Get("foo")]
    partial Task FooAsync();
}

var api = new MyApi("https://api.example.com");

which avoids needing to use reflection. It also means the user can add their own concrete helper methods. However, there are some downsides: users like being able to define interfaces from an abstraction standpoint, and it would make the README significantly more complex (each code sample needs duplicating).

Continuing to use interfaces shouldn't be too bad: we can emit an assembly-level attribute saying which generated type implements which interface, which we can pick up on at runtime.

Another consideration is what will happen if the source generator isn't working. With a partial class, the compilation will fail. With interfaces, we simply won't get an implementation generated, and we won't know until runtime. I don't know whether this might be an issue in practice.

We'd also want to keep the RestClient.For syntax for interfaces, presumably. However, I'd want both source generation and runtime IL generation to be opt-in, and allow both to run side-by-side, so RestClient would need to be defined in a central place, and do nasty things to make sure that it did runtime generation if necessary, and if possible. That could cause problems.

Identifying which partial classes / interfaces need implementations generated could be tricky: we don't currently have a guaranteed type-level attribute, so we'd have to go looking through all methods of all types.

Json.NET

Json.NET is currently a mandatory dependency. That makes the simple case simpler, but means people who don't want to use it still have to take a pointless dependency.

This never used to be much of a problem: most REST APIs are json, and everyone used Json.NET. However System.Text.Json is now a thing, and forcing people who want to use System.Text.Json to take a dependency on Json.NET is wrong.

However, we still need to support people who want to use Json.NET.

Backwards compatibility

Major version bumps are a good excuse to make breaking changes. I'm OK with breaking changes in principle, but they must be compile-time breakages (I don't want things failing only at runtime), and there should ideally be guidance on how to solve it (e.g. use of Obsolete attributes).

Changes

I want to move all common types (attributes, RequestInfo, Requester, etc) to a RestEase.Core assembly. This is necessary to let source generation and runtime IL generation work side-by-side. These would stay in their current namespaces. It's unclear what would happen to RestClient.

Currently the various serializers and deserializers are separate properties on RestClient. In order to let the user opt into the Json.NET (de)serializers easily, these would have to be grouped on a new object. Then the user could do new RestClient() { Serializers = new JsonNetSerializers() } or similar. The JsonSerializerSettings would have to be removed from RestClient as well.

We'd need a RestEase.Json.NET NuGet package which contains the Json.NET (de)serializers.

Presumably we'd want two more NuGet packages, one for the source generators and one for the runtime IL emission (they can't be the same package, because the source generators shouldn't have a dependency on ILBuilder).

It's unclear what should happen to the current RestEase NuGet package. Maybe this should continue to contain the IL emission code, or maybe it should be a backwards-compatibility meta-package which references the IL emission NuGet package, as well as the RestEase.Json.NET NuGet package.

Other Changes

It's also a chance to tackle #19, and not assume that responses are always strings. This needs more thought.

Hellevar commented 4 years ago

They've introduced preview of Source Generators

canton7 commented 4 years ago

I've been mulling over this further, and I'm leaning in favour of keeping things as a single package. This would include the source analyzer and the S.R.E implementation. The S.R.E would be used as a fallback in case the source generator fails, or isn't available (on platforms which support it).

This makes life a lot simpler: people don't have to make informed decisions at the point of installing the library (which is a hurdle). They install the RestEase package, and if their toolchain supports source generators, they get nice compile-time error messages; if it doesn't, things work as before (except on Xamarin.iOS, where they'll get a nice exception message pointing them at the docs for source generators).

This does mean we'll ship a S.R.E implementation to platforms which don't support it, but that's probably OK. .NET Standard 2.1 includes S.R.E (and the latest Xamarin versions target .NET Standard 2.1), so we won't have an extraneous dependency on the S.R.E NuGet package

There is the thorny issue of what to do about the Newtonsoft.Json dependency, though. Intuitively, we should target System.Text.Json by default, but 1) that's only supported on some platforms, so we'll get different behaviour depending on the platform, and 2) that ends up going down the route of silently switching people from Newtonsoft.Json to System.Text.Json, which is going to break things for some people. Therefore I think we have the following options:

  1. Deprecate RestEase (NuGet supports this now) and switch to RestEase2. The migration notes tell people what to do in order to keep using Newtonsoft.Json (whatever that may be).
  2. Detect whether Newtonsoft.Json is loaded and automatically switch to it. I don't like this at all: installing an unrelated package will silently change the behaviour of RestEase.
  3. Don't target any json library by default, and force the user to explicitly opt into one. We'd want a nice way to opt into a library, such as an attribute on the interface (which is something I've been thinking of anyway: the current approach of putting that config on the RestClient removes it from the code which actually uses it). We would need logic to detect when the user is trying to use an interface which needs serializer support (check for method return types which need deserializing; SerializationMethod, etc), and throw a suitably informative error message. We will break some users this way, however.
  4. Something similar to what I was thinking of before: RestEase becomes some sort of compatibility package which references the "main" RestEase NuGet package, and also the Newtonsoft.Json RestEase package. It needs some way to tell the main RestEase package that the Newtonsoft.Json package is available however, and this gets ugly.
canton7 commented 4 years ago

A slight evolution: the SourceAnalyzer will be in its own package -- you can't bundle a library and source generator in the same package. The rest will stay the same: I'm planning to ship the S.R.E implementation in the RestEase package, then people can install the RestEase.SourceGenerator package to get the source generator. RestClient will try to get a source generator generated implementation first, then fall back to S.R.E on platforms that support it.

This means that I don't need to cut RestEase 2.0 in order to release the source generator. I do need to add more types to support the soruce generator, so there'll be a minor version bump, but that's it.

I'll open a new issue when the source generator is testing. It's getting there -- ironing out the edge cases. Obviously it won't be released at final until the design of source generators is released, probably with C# 9.

canton7 commented 4 years ago

The source generator preview is now out, see #158

canton7 commented 3 years ago

The source generator has shipped, as a separate package. This means that it's opt-in (and if there are teething problems, noone's forced to use it). In the future, I'm tempted to have a RestEase meta-package which references a bunch of things, including the source generator.

170 is probably something that should also be addressed for 2.0.

canton7 commented 3 years ago

The SG also means that there's little point in a separate RestEase.Core library: the SG'd code needs to reference lots of RestEase types, and so these need to be available in all assemblies.

The main thing I'd currently like to see in v2 is a nice way of opting into json.net, Polly, etc. Ideally this means some way of adding extension methods into the RestClient.For call, naively something like:

T api = new RestClient(...).UseNewtonsoft(settings).UsePolly(policy).For<T>();

That's nice, but it's a bit wordy, especially if the user's primary way of getting instances of T is calling into RestClient.For directly (i.e. they haven't wrapped it into their own factory). There are two possible approaches I can think of here:

  1. Make the user create and configure a factory which is used to generate instances of T. This raises the barrier to entry a bit though, and requires that the user has a central place to set these sorts of things up, and a way of distributing said factory to all of the places which need RestEase, which isn't necessarily a given.
  2. Do something with attributes

The attributes approach feels nice: an interface which reference models which are inherently either json.net or S.T.J, so configuring this at the interface level makes sense. However it then gets harder for the user to set their own json.net-related options, or pass their own Polly policy, or...

So something like this is unlikely to be flexible enough:

[UseNewtonsoft]
public interface ISomeApi
{
}

Something like this might fly:

[RestEaseFactory(typeof(Factory))]
public interface ISomeApi
{
}

public class Factory : IRestEaseFactory
{
    public RestClient GetClient() => new RestClient(...).UseNewtonsoft(...);
}

That's a bit more headache though, and I don't want newbies to have to add that much boilerplate just to get up and running. I also don't know whether the complexity is worth the effort: maybe everyone who uses RestEase in a complex enough scenario that they need to be configuring it already has DI set up, or is wrapping it up in some sort of factory method.

A hybrid approach, where both are allowed, might make sense (so e.g. public void ConfigureClient(RestClient client) => client.UseNewtonsoft(...) above), with the options from one overriding the options from the other.


I'd also like the use the opportunity to make some breaking changes:

  1. Get rid of [Obsolete] members
  2. string return types are no longer special, and get passed to deserializers. Maybe add an attribute to get a raw string return.
netclectic commented 3 years ago

Personally, I'm not a fan of the use of attributes in this way. I prefer to see exactly what's happening at the point of use. For this scenario I like the fluid api style, T api = new RestClient(...).UseNewtonsoft(settings).UsePolly(policy).For<T>();

So long as you have sensible defaults to fallback to then the user can just use RestClient.For as normal unless they require specific non-default behaviour, in which case I think its perfectly reasonable that you need to do a little extra setup work.

canton7 commented 3 years ago

Agreed -- the idea with the attributes was to still configure RestClient using that style, but put those method calls in a central place rather than forcing the user to repeat them every time they create a new instance.

So RestEaseFactory (or whatever it's called) would refer to some type, and that type would define a method which contains calls to exactly that same fluid API. A lot like the Startup class in asp.net

canton7 commented 3 years ago

There's also the matter that people are publishing libraries containing RestEase interfaces. It might be nice to let them specify what serialization to use with those, rather than making them add documentation telling the user what configuration they need to do.

netclectic commented 3 years ago

But on the flip side, as a consumer I would want to choose which JSON library I was going to use.

canton7 commented 3 years ago

If you're using models which someone else has written, you'll need to use the same json library as the models are intended for. If they're decorated with json.net attributes, they won't work with S.T.J!

CollinAlpert commented 1 year ago

This might also be a good time to drop .NET 5 support and move to .NET 6, since .NET 5 is EOL. Although, I don't even think you'd need a major version to do this.

canton7 commented 1 year ago

I don't think there's any need to drop support for older TFMs, unless we need some API which isn't available there? And since we're supporting Framework for the forseeable future that seems unlikely.

Supporting a TFM as a library just means that we don't need any of the APIs from newer TFMs. We'll of course run on whatever runtime the main application is running on.

Regenhardt commented 1 year ago

Is there a way to use RestEase with STJ yet?
I'm trying to implement WebAuthn in a Blazor application using the passwordless Fido2 lib's models, which use STJ attributes and can therefore not use Json.NET as it seems, so I can't communicate the models with the server via RestEase yet.

canton7 commented 1 year ago

You can write your own serializer which uses STJ - the only side is you'll bring in json.net as a dependency, even though it's unused.

There's also a third party fork of RestEase on nuget, although I can't vouch for it