Skip to content

What is the benefit of this client? #15

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

Closed
ohadschn opened this issue Jul 24, 2016 · 7 comments
Closed

What is the benefit of this client? #15

ohadschn opened this issue Jul 24, 2016 · 7 comments
Labels
status: help wanted requesting help from the community type: question question directed at the library

Comments

@ohadschn
Copy link

ohadschn commented Jul 24, 2016

I apologize for the blunt question, but why not just use HttpClient?
What's the difference between sgClient.your.api.get() and httpClient.GetAsync("Your/API")?

The former might be confusing to C# developers, as the API calls are not truly type safe and are pretty much equivalent to the string...

@thinkingserious thinkingserious added status: help wanted requesting help from the community type: question question directed at the library labels Jul 25, 2016
@thinkingserious
Copy link
Contributor

Part of the question/answer is here: sendgrid/sendgrid-csharp#261 (comment)

These are valid points as well as in the other thread. I'll leave this ticket open so that others may weigh in.

Some of the reasons we decided to create our own client in a separate repo:

  1. The SendGrid API only needs a subset of the functionality of HTTPClient, this was an attempt to simplify and only expose those needed functions.
  2. All of our libraries have the same simple input/output structure, so there was some thoughts of consistency and maintainability across the 7 libraries. (but the needs of particular communities outweigh this and we hope that those who are experts in each language will weigh in, as you have, to help guide how we can make this library better for you)
  3. Having it in a separate repo allows people to have a simple interface to REST APIs without the coupling to the SendGrid library
  4. We want people to use whatever HTTPClient they want, so this was a first step in decoupling the client from library

@ohadschn
Copy link
Author

Hey Elmer,

Thanks for your response. Here's my 2 cents:

  1. HttpClient is really pretty simple, as you saw in the other thread it's 3 lines of setup (creation, base address, authorization) and then it's one line for each API call e.g. client.PostAsync(...). I think it can even be made shorter by having the Mail class comply with DataContractJsonSerializer (essentially by using DataMemberAttribute) therefore allowing the super simple client.PostAsJsonAsync("mail/send", mail).
  2. I think a C# developer expects a library to look like a C# library, a JS developer expects it to look like a JS library, etc. Otherwise we'd all be using the same language :)
  3. Assuming the client is required, I complete agree with the separate repository approach. I'm currently challenging the first part of that equation though :)
  4. Like I said in the other thread, I believe the vast majority of users will just use the vanilla HttpClient. And if someone is bent on using their own HTTP client they always have the REST API which is really simple, and still enjoy the helper classes (as I demonstrated in the other ticket). You can also let people pass in their own HttpMessageHandler. Finally, I'm not sure I understand how this client helps in said decoupling (a simple code example would be great).

@thinkingserious
Copy link
Contributor

Thanks again for contributing more great feedback!

Please keep it coming.

Regarding 1, we need to also account for the other verbs, so we are using SendAsync with a request object, but I get your point; that call looks beautifully simple.

I'd love to simply have client.send(mail), which I think we will achieve with the helper once we get to iterating on that again. The user should not have to worry about what the actual path to the endpoint is, though I think they should have that option for flexibility, especially since it will be some time before we have helpers for all the major use cases.

Fully agreed on point 2. And for C#, you are playing a big part in helping us get that right for this language. So thanks again!

I'm not sure I follow point 3. I think you mean in regards to the simplicity. If so, agreed, there is work to be done there, but we have only just begun :)

Regarding 4, I'm not sure yet either. We need to dig a bit deeper and hopefully others in this community will chime in.

In any case, before we make our next iteration on this component of the library, we will create an issue and solicit further feedback and your feedback will be incorporated.

Perhaps you may want to submit an issue that proposes to remove the dependency from the SendGrid and just use HTTPClient within the library. That might hopefully spark some additional feedback.

@ohadschn
Copy link
Author

ohadschn commented Jul 29, 2016

I'm not saying there's no benefit for a client of your own the user can use without caring about the underlying REST calls, that's actually pretty cool. I'm just saying that IMO dynamic isn't the way to go here. I was thinking more about the lines of "contain and delegate" using an underlying HttpClient, with an optional user-supplied HttpMessageHandler. Something like:

public sealed class SendGridClient
{
    HttpClient m_client;

    public SendGridClient(
              string apiKey, string baseAddress = null, HttpMessageHandler handler = null)
    {
        m_client = new HttpClient(handler ?? new HttpClientHandler())
        {
            DefaultRequestHeaders = 
            { 
                Authorization = new AuthenticationHeaderValue("Bearer", apiKey) 
            },
            BaseAddress = new Uri(baseAddress ?? "https://api.sendgrid.com/v3/")
        };
    }

    // As the next phase you could wrap HttpResponseMessage in a class of your own too
    public Task<HttpResponseMessage> SendMailAsync(Mail mail)
    {
        return m_client.PostAsync(
                "mail/send", new StringContent(mail.Get(), Encoding.UTF8, "application/json"));
    }

    //... implement more SendGrid-specific methods ...
}

As for the missing methods, in my opinion you should let the users fend for themselves with the REST API until the official client support arrives - it's really quite simple. Anything else would feel half-baked, like a "mix" of official and custom methods. If you still want them to use your client I see two options:

  1. Unseal SendGridClient and make m_client a protected property with a private setter. Then a user could simply inherit and implement his own GetCategories or whatever.
  2. Add "generic" methods such as public Task<HttpResponseMessage> PostAsync<T>(string path, T content) { return m_client.PostAsJsonAsync<T>(path, content); } but I like this option even less since you're exposing the REST API which is what you've been trying to hide to begin with.

@ohadschn
Copy link
Author

ohadschn commented Jul 29, 2016

BTW like I mentioned in the other thread you could also enrich the helper classes for example:

class Mail
{
    //...
   Task<HttpResponseMessage> SendAsync(string apiKey)
   {
       return new SendGridClient(apiKey).SendMailAsync(this);
   }
}

You can also do this with extension methods if you don't want the helper classes to be aware of the client.

@thinkingserious
Copy link
Contributor

Hi @ohadschn,

I'm not sure if you saw the new v9 of our SDK, but just wanted to point you there as I close this issue. Our SDK is no longer dependent on this library (and is now no longer using dynamics) and we will leave the dynamic functionality here in this SDK.

Thanks!

@ohadschn
Copy link
Author

ohadschn commented May 1, 2017

@thinkingserious that's great news, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: question question directed at the library
Projects
None yet
Development

No branches or pull requests

2 participants