RicoSuter
Add option to use RestSharp
I would prefer this also. There are lots of problems with HttpClient when used incorrectly (non single instance). So we always use RestSharp now and have our own wrappers around that.
Forked the code, so gonna take a look at implementing this.
BTW, the option to use a non single instance of HttpClient really need to be removed. You should not be using local instance of that in production code.
I started on this and added the UI option, but after using the HttpClient version not sure if it's all that necessary. Since these API's are not using cookies, using a shared HttpClient instance is possible.
I do think the code should be modified to completely remove the option to have a non-shared HttpClient due to the issues related to constantly creating and disposing of those:
https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/
So for now I am going to start with the HttpClient version and see how it works out. The main thing is using interface files for the client generated we can mock it out nicely, which is the thing I care about most.
I do think the code should be modified to completely remove the option to have a non-shared HttpClient due to the issues related to constantly creating and disposing of those:
I don't know how it's implemented in NSwag, but I'd say that there's nothing wrong in using http clients from a IHttpClientFactory (which is made specifically for that). See https://docs.microsoft.com/en-us/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests
Conceptually, you're creating and destroying instances of HttpClient, but the connections are handled by the IHttpClientFactory.
I agree, however, that new HttpClient is bad and must be avoided.
Well NSwag does not use IHttpClientFactory, and honestly it should not. It should be up to the caller to create an IHttpClient instance and inject it into the client class via the constructor, which by default is how NSwag will generate the client. If the client is generated by IHttpClientFactory (a great idea by itself), or a static HttpClient instance, it ends up used in the NSwag generated client the same way.
So what I propose is we completely remove the option to generate client code that creates and manages HttpClient instances internally using 'new HttpClient'. We should not give folks the option of generating client code that in the long run is going to cause them bugs and heartache.
More than happy to make these changes if people agree it is worthwhile.
I agree it is worthwhile, but that's another topic and a breaking change. I'd suggest that we open another issue to discuss that.
Maybe we should deprecate the feature instead of removing it immediately ?
Sure, deprecating it initially would make sense. Not sure how to cleanly get the message to the user about it though? If it's removed completely as soon as someone regenerates the class with the newer version, it will break their code. So it's easy enough for the to either correctly fix it at that time, or keep using the older tools until they fix it.
Well this original ticket was about adding RestSharp support, but we ended up just using the default HttpClient code with a single instance of that class (the correct way). I suggested that the code generation be changes to not generate it's own internal HttpClient instances. This will break existing code, but IMHO it's simple for someone to fix to work around the code breakage, and it will make them correct their code to use HttpClient correctly as a single instance.
This should not be an optional 'switch' IMHO, it should cause the code using it to fail until they fix it.
Initially you could make those interfaces as deprecated but that generally causes code to fail to compile anyway.
