Open ericdunn opened 5 years ago
Without creating a full pull request, something like adding an HttpMessageHanlder to ShopifyService's constructor may do the trick :
private static IRequestExecutionPolicy _GlobalExecutionPolicy = new DefaultRequestExecutionPolicy();
private static JsonSerializer _Serializer = new JsonSerializer { DateParseHandling = DateParseHandling.DateTimeOffset };
private static HttpClient _Client { get; set; }
private IRequestExecutionPolicy _ExecutionPolicy;
protected Uri _ShopUri { get; set; }
protected string _AccessToken { get; set; }
/// <summary>
/// Creates a new instance of <see cref="ShopifyService" />.
/// </summary>
/// <param name="myShopifyUrl">The shop's *.myshopify.com URL.</param>
/// <param name="shopAccessToken">An API access token for the shop.</param>
protected ShopifyService(string myShopifyUrl, string shopAccessToken, HttpMessageHandler handler = null)
{
_ShopUri = BuildShopUri(myShopifyUrl, false);
_AccessToken = shopAccessToken;
_Client = handler == null ? new HttpClient() : new HttpClient(handler);
// If there's a global execution policy it should be set as this instance's policy.
// User can override it with instance-specific execution policy.
_ExecutionPolicy = _GlobalExecutionPolicy ?? new DefaultRequestExecutionPolicy();
}
Implementing it as null by default wouldn't break anybody, but would allow a bit more flexibility.
It's a bad practice to instantiate a new HttpClient for each request. HttpClient must be long lived. We could potentially make the static HttpClient setter public. Or somehow use the recently added HttpClientFactory designed to provide more flexibilty.
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-2.1
You're right about multiple HttpClients lol. I think the clientfactory route is probably better, since you then remain in control of the HttpClient. Either or for me works really!
Only problem there I guess would be the need to up the min support to netstandard2.1, and drop support for anything lower, or figure out a clean way to use preprocessor directives. It can be done, and is probably the right thing to do (using the factory vs handling the client yourself), but it's not a quick fix. I'll see about creating a pull request and see what that looks like.
That's likely a little aggressive to move everyone to my beloved netcore2.1.
Could this be handled with a global setting that just creates a new httpClient right when you set it?
Like how the execution policy is handled, but for all?
I agree with you there. For now, I just override ExecuteRequestAsync and set things up myself. Not ideal, but at least it unblocked me. I'll try to think of a good solution over the weekend.
Personally I like the mimicking the execution policy.
You could have something that would return either an HttpClient from there, or an HttpClientHandler (although this approach would probably mean instanciating a new HttpClient everytime to make sure to use the provided handler). Then there could be a default that just uses the status quo.
Something to think about for sure. Thanks all!
Interesting use case! I also like the execution policy suggestion. What about making the underlying HttpClient virtual, so you can extend each service that you use and then set your own HttpClient in the constructor?
Edit: woops nevermind, forgot that the HttpClient is static. That wouldn't work.
You could make it protected however, that would work in my specific case, but wouldn't really allow anybody to set their own proxy details without at least creating their own version of each service they want to use.
Just thinking out loud, this package is constructing HttpRequestMessages for each request and passing those to the HttpClient. I wonder if there's a way to set the proxy on those messages. Then we could add a global or instance-specific function to give a proxy to the service, and the service can pass that to the message.
I'd also like to explore adding the client factory in 5.0, where we can add support for netstandard2.1
Hey guys,
I just updated to the latest version of ShopifySharp, and based on a previous commit, you are now using HttpClient instead or Flurl. I don't mind removing that dependency, but this did break my app in the sense that I was setting proxy details using Flurl's FlurlHttp.GlobalSettings , so it worked across the app. Not the best, but it worked.
I was wondering, now that we're using HttpClient, how would one manually set proxy details? I can't rely on what's configured on the system and have to manually set it.
Any ideas?
Thanks! Eric.