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

BREAKING CHANGES #317

Closed
thinkingserious opened this issue Sep 12, 2016 · 29 comments
Closed

BREAKING CHANGES #317

thinkingserious opened this issue Sep 12, 2016 · 29 comments
Labels
status: help wanted requesting help from the community type: question question directed at the library

Comments

@thinkingserious
Copy link
Contributor

As a result of your feedback and as a continuation of the execution of our long term roadmap, there will be breaking changes coming soon. The last time we had a major breaking change, it was for the implementation of v3 Web API support. We announced those changes here on GitHub along with some instructions on how to test and provide feedback.

The feedback we got was amazing, but we didn't quite get the amount or thoroughness of feedback we were hoping for.

We want to continue improving this iterative process, so we are reaching out to you for feedback in order to determine the optimum way to move forward as this library is designed to be community driven, SendGrid led.

Please take a moment to share your thoughts with us.

Following are some ideas we are considering, we will likely choose one from Execution and one from Communication, but not all of the items below.

Execution (for large changes):

  1. Utilize the pre-release functionality in GitHub to gather feedback and contributions to a specific branch
  2. Communicate through a dedicated GitHub issue that references that pre-release branch (e.g C# Dynamic Update)

Communication:

  1. Continue using GitHub issues
  2. Opt-in email newsletter for announcements
  3. Opt-in email mailing list
  4. Dedicated social media account (e.g. @sendgrid_libraries)

How do you prefer to get these announcements? Is this too much? Too little? Please let us know!!

As always, we are listening carefully and are looking forward to working with you.

Note: We will always follow the semver.org convention of using major point releases to signify breaking changes. Please DO NOT auto-update your dependencies. It is important to take a look at the CHANGELOG or releases to find out how the breaking changes will impact your code.

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

xt0rted commented Sep 12, 2016

👍 to pre-releases as long as they're tied to a NuGet release and then CI builds could go to MyGet1.

Azure/azure-webjobs-sdk and NancyFx/Nancy use similar deployment setups with CI builds on MyGet and periodic betas to NuGet which is very important to me when needing to easily get a bug fix/feature into my project before an official release since I can just grab the CI build containing that fix.

I like the idea of an email that contains announcements and overall status updates since it's easier to consume and then using GitHub issues for change proposals and feedback gathering (using 👍 👎 along with comments). Is there a dev blog that these types of things could be published on as well? I've been adding various RSS feeds to Slack for easier notification for some of the services I'm using.

I'm currently staying with v6.3.4 until dynamics are gone so I'll be following along closely with the next major release.

1 If you use AppVeyor for builds they have a built-in NuGet feed that could be used for this though MyGet might be easier to discover since you could list the feed in their gallery. You can also set the build config to publish to NuGet based on tags/releases (including pre-releases) which can remove any manual process that may be in place. I'm using this setup in two of my projects and it's great. Travis may support this too but I don't have any experience with it, just AppVeyor.

@thinkingserious
Copy link
Contributor Author

Thanks for the great feedback @xt0rted! I've added your vote to the dynamic replacement ticket.

@OG84
Copy link

OG84 commented Sep 13, 2016

+1 for github issues and Twitter announcements

@chadwackerman
Copy link

chadwackerman commented Nov 12, 2016

A quick data point from the trenches: I just updated 20+ Nuget libraries on a project that's been running untouched in the cloud for six months. This was the only library that had major compatibility issues.

I cannot imagine why something as braindead as SendGrid.SendGridMessage would need to be changed, let alone deleted as part of a refactor. As you continue to innovate with this library, please remember that some people are just trying to send some emails.

Edit to add: I spent an hour on this and the V3 stuff is a major step backwards. I had five lines of code that used to work great and now it's a total mess. You call addresses "Email"? And renamed Message to "Mail"? System.Net.Mail figured all this out 10+ years ago.

Three layers of totally needless dynamic stuff and you even turned message.DisableClickTracking() into twenty lines of code, objects on top of objects. That's when I threw up my hands and reverted.

@thinkingserious
Copy link
Contributor Author

Thank you for your feedback @chadwackerman,

Apologies for the poor experience, we are working hard to make things right.

We appreciate that you took the time to write us and also for reverting back to v2 and continuing to support SendGrid. We are taking all feedback into consideration as we continue with the v9 development of this SDK.

Here are some specific resources in case you would like to dig deeper:

  • You may continue using the v2 API as described here.
  • The mail helper, the main interface to the mail send endpoint, is currently being refactored and you can follow the progress here. See the v9beta tags.
  • Discussion around naming conventions is here.
  • Dynamic objects are being removed here.

@chadwackerman
Copy link

chadwackerman commented Nov 14, 2016

I appreciate this but I'd like to take a moment to point out that so much of this represents a few hours work by a skilled .NET developer and throwing it to the community is a little goofy. I've watched so many companies fail at this exercise because while they're distracted getting positive, social feedback from a small minority of users who have little else to do but dick around on GitHub all day, the enterprise and professional developers who keep them in business are looking at all this churn and rolling their eyes.

Your goal is to sell SendGrid subscriptions. You shouldn't aspire to be the mayor of SendGridTown. Mayor doesn't pay particularly well and as this very comment illustrates, most people only contact the mayor when they're pissed off about something.

There's absolutely NOTHING about your service that requires community feedback at this scale. You moved a few strings from the HTTP header to the HTTP body, put curly braces around them, and replaced "x-" with "". Why do I even know your GitHub handle? That itself indicates a massive failure.

Absolutely nobody asked you to change this stuff to begin with. But 10,000 upvotes for "don't screw around with the published API" isn't atop any GitHub issues list I've ever seen. Because that wouldn't make GitHub money... dig?

@thinkingserious
Copy link
Contributor Author

@chadwackerman,

Thank you for the continued feedback.

Though we disagree on several of these points, I think we share the same objective, that this SDK and the API should make interacting with SendGrid dead simple. Based on our current roadmap, I believe we will get there.

Thanks again for taking your valuable time to express your concerns. We are listening.

@jduncanator
Copy link

jduncanator commented Nov 15, 2016

I somewhat agree with @chadwackerman (even if I don't agree with his tone). The API library in v2 was a much better implementation of the SendGrid API than what is currently in the v3 library. I have no idea who's idea it was to (ab)use dynamics the way they currently are, but as soon as I saw that I looked for alternatives. There is absolutely no excuse to develop a dynamically-typed implementation in a language that is statically-typed. It's like developing the entire library in JavaScript and then embedding a JavaScript Engine in .NET to run the library.

Whilst I appreciate there is progress in moving away from the dynamically typed interface, I have no idea why the interface even changed between v2 and v3. If I had my vote, I'd say keep the API interface you have from the v2 library, and implement it in a way that works with the v3 SendGrid API, however something tells me it's too late for that. Having a type that is explicitly SendGrid's (eg. SendGridMessage) as opposed to an Email type, a Content type (etc) means that I no longer have to carefully manage which namespaces I import (the type Content conflicts with numerous built in types in .NET).

@thinkingserious
Copy link
Contributor Author

@jduncanator,

It's not too late, it's what we are working on now with this branch (your feedback would be greatly appreciated): #350

And we'll be merging that with the removal of the dynamic dependency here: #356

@jduncanator
Copy link

@thinkingserious I guess what I'm trying to achieve is a more .NET like approach to the API implementation. The v2 library very closely follows how the out-of-the-box .NET Mail implementation works, which means it's very easy for developers to swap out their existing SMTP email provider with your SendGridMessage with very little refactoring needed. I'll leave some feedback on #350.

@thinkingserious
Copy link
Contributor Author

@jduncanator perfect, thanks!

@jkewley
Copy link

jkewley commented Nov 16, 2016

Along those lines, you need to get the Azure documentation updated to reflect the current coding approach.

I figured I'd whip out a quick console app and ship it as a webjob tonight in no time, given the lengthy partnership that SendGrid has enjoyed with Azure as the go-to for outbound email. It turns out that the official Azure article that explains the 101 approach was written with the v2 API almost a year ago, so it's a source of confusion rather than the quick answer I was looking for. Every other article I've come across thus far that was written this year makes use of an API key rather than the credentials that one secures from Azure.

How about a code sample in this repository that shows a current Azure implementation if it's too much trouble to maintain the official Azure docs authored by SendGrid?

@chadwackerman
Copy link

I think they want to pretend the V3 thing doesn't exist at this point. If you just want to send a password reset email that doesn't wind up in spam folders, good luck figuring out how to turn off all the SendGrid "enhancements." (I really don't need read tracking for password reset emails, guys.) SendMailMessage.DisableClickTracking() is now 30 lines of code using mysteriously named dynamic objects and no autocomplete.

Per above, V2 is supported, works fine, and you should wait for V9. (Yes, version nine of an API that sends email messages.)

@jkewley
Copy link

jkewley commented Nov 16, 2016

Which version of the SendGrid Nuget package has the object model that most of the documentation on the Internet refers to (which I'm guessing is the v2 model)? SendGridMail and such..

@thinkingserious
Copy link
Contributor Author

@jkewley: https://github.com/sendgrid/sendgrid-csharp/blob/master/TROUBLESHOOTING.md#v2

jkewley added a commit to jkewley/azure-docs that referenced this issue Nov 16, 2016
…ersion

In this issue: sendgrid/sendgrid-csharp#317? the author of the SendGrid package points to this page https://github.com/sendgrid/sendgrid-csharp/blob/master/TROUBLESHOOTING.md#v2 which says that version 6.3.4 of the Nuget package is required to be able to work with the SendGrid object model demonstrated in this article
@mbernier
Copy link
Contributor

I am jumping in late here, but wanted to share some insight into what happened just in case it helps.

SendGrid has had the v2 API for a very long time (5+ years). It was time to create v3, but the API world had changed so drastically that we had to be willing to shake things up a bit.

Best practices would say that we should keep all the interfaces as close to the same as possible, however the API endpoints changed from a web API to a RESTful API. This means that the interfaces changed completely. This includes the information that was shared, the structure, the responses, how the API responds... everything about the API is different. Typically, with a new version the differences would be minor. This was a complete overhaul. It was desperately needed, to meet the requirements and technology of the community of API consumers as well as all the normal internal reasons that you would do a project like this.

At the same time, we had to deal with the reality that our libraries were not scalable in the way that they were built. They also did not cover anything more than our mail endpoint. While most of our customers are only using the mail endpoint, we have so much more functionality that our customers wanted/needed/didn't know about, that it was worthwhile to look at how we could add all of those endpoints to the libraries as well.

We produced our MVP and it was a complete change, much like our endpoints were a complete change.

The thing to remember here was that no one using v2 has to upgrade to v3. Our endpoints in v2 aren't going away any time soon and will continue to scale and meet our customer's needs very well. V3 will get our newest features, to be sure, but v2 is a solid platform that has served SendGrid and our customers well for 7 years!

For our other languages, we did very well and received a great response with our libraries. For C#, we quickly learned that the decisions we made (dynamic, snake case, and others) were a total mistake. We looked to you to let us know this, and boy did you all! Sincerely, thank you so much for your input and your thoughts.

Now, we are working hard to completely rewrite our C# library to meet the expectations of our community and to stop doing silly things like using dynamic. It will be completed soon and it will be available for more input. We are excited about these changes and the things we will learn from you all once it is done.

I really hope that this information helps. I am also personally sorry for any trouble that these changes caused you. As the person responsible for Developer Experience (it is literally my job title), I feel deeply for these missteps and the trouble they cause. With your feedback, information, point of view, and use cases we will move this thing forward and work to solve as many email-related problems as we can for your business.

@jduncanator
Copy link

jduncanator commented Nov 22, 2016

@mbernier Whilst I agree with a lot of the points you've made, I completely disagree with the statement that the public facing APIs had to change. I've maintained very large API implementations over the years that have gone through v1, v2, v3, and so on, and very few times have I had to change the public facing API in a way that was not backwards compatible. There was absolutely nothing wrong with the v2 interface (bar a few missing parameter options).

The public facing interface should not reflect 1:1 the implementation of the website API. There should be a clear distinction between the public interfaces you expose to consumers of your library, and the back-end code you maintain and changes as the web API does. You should be abstracting away any of the RESTful-ness of your application into a common, standardized format that most C# developers will understand and expect. A change in your website API should not result in a change in your public facing interface, except for new features which should only be additive and not affect in a breaking way the existing features.

In order to make your v2 public interface work with your new v3 API, all it would have taken was a few days work to implement a new backing implementation. Done correctly you should be able to change the entire website API with everyone being none the wiser. (And this is, in fact, what we have done to the v2 SendGrid API in our internal projects).

I understand there will be changes the have occurred from v2 to v3 that needed you to break backwards compatibility, but nothing I have seen yet should have warranted the changes that have been made from v2 to v3.

@chadwackerman
Copy link

@mbernier agree totally with @jduncanator's comment. But I also want to point out that doing this live on GitHub is effectively taking the bad decision then cranking up "Yakety Sax" on the stereo.

Watching the totally unmanaged attempts to right this ship is cringeworthy. It's a few days work for a single engineer, tops. Only on GitHub could something so simple turn into months of effort involving dozens of people.

@thinkingserious
Copy link
Contributor Author

Thanks for the continued input everyone, we greatly appreciate the time you took to do so.

@panterlo
Copy link

panterlo commented Dec 6, 2016

@thinkingserious I have read these comments and we are a significant customer to SendGrid having over +100k e-mails a month. While I myself wrote the initial integration to v2 I enjoyed the API but v3 is a misuse of dynamic in a strongly typed language like C#. The intention of the dynamic keyword and use is not to use it like you are doing in the API. You guys should go back, rewrite the full API to be a similar as possible to v2 laying over the need of additional classes for v3.

For me this change shows that SendGrid doesn't understand how to deliver solutions that really works in large scale when making such significant changes. For me we are moving away for SendGrid the next coming days.

@thinkingserious
Copy link
Contributor Author

thinkingserious commented Dec 7, 2016

Hello @panterlo,

We are almost there: https://github.com/sendgrid/sendgrid-csharp/tree/net-standard

This solution has the dynamic dependency removed and works with .NET 4.5.1+, .NET 4.6.* and .NET Core. Once I can get it to play nice with Travis, I'll be reworking the Helpers to provide an experience much closer to v2.

Thank you for your continued support and feedback!

@taspeotis
Copy link

+1 for dynamic being the wrong tool for the job. And:

dynamic response = await sg.client.mail.send.post(requestBody: mail.Get())

feels very lazy, why does mail have a post method with a requestBody parameter? Don't wrap a thin abstraction layer over HttpClient - that's just indirection. No points for post vs. PostAsync, either. And if I had to guess ... post doesn't have a CancellationToken overload.

But I also want to point out that doing this live on GitHub is effectively taking the bad decision then cranking up "Yakety Sax" on the stereo.

I think SendGrid should provide an idiomatic .NET library, and it's really not that hard, so how'd we get here? But I can't criticise them for being open. Being misguided perhaps.

@thinkingserious
Copy link
Contributor Author

@taspeotis,

Thanks for the feedback!

This work is already underway and we would love your feedback on our progress: #331

@rakker91
Copy link

I'd like to echo many of the comments that have been written already. I looked to upgrade a client of mine from v2 to v3 and realized that I'd just be better off using RestSharp and connecting directly. I REALLY don't want to spend several hours of valuable time reworking 9 lines of code into many, many, many more lines of code, just to send simple non-marketing e-mail messages. On one project, I started the process, then abandoned it entirely and just switched to using smtp.sendgrid.com and the .net MailClient. On another project, I'll just be reverting to v2 because the cost of implementing v3 isn't worth the pain.

I understand breaking changes, but if I wanted to use rest endpoints directly, I'd use RestSharp. C# is a strictly typed language.

What I'd like to see (and what I'm hoping v9 is, although I don't have time to go investigate) is an object model that exposes the functionality and HIDES all of the REST functionality that's easy to discover (email has been around for a VERY long time--I shouldn't have to reference documentation just to send a simple e-mail). If you want us to know about advanced features, expose them through the api, but fundamentally, I don't need and am not using those advanced features. If the API is solid, when I want those features, I'll look to SendGrid first ('cause I already know you have them), unless, of course, the cost of integrating SendGrid is too high. Then I'll look elsewhere.

We send more than 100k e-mails a month with Sendgrid, and it works great. These aren't marketing messages, these are messages communicating things like status of projects, project results, etc. with our customers and vendors.

Don't make us regret our choice by making what should be 9 lines of code hundreds of lines of code instead.

@Jericho
Copy link

Jericho commented Dec 17, 2016

@rakker91 May I suggest StrongGrid, a strongly typed library (absolutely no dynamic ), fully async, covers SendGrid's entire API (including email, or course, and also creating lists and segments, managing contacts, etc.) with easily discoverable model (thanks to intellisense). It also includes a parser for events webhooks and inbound email webhooks.

With StrongGrid, sending an email is as simple as this:

var client = new Client(apiKey);
await client.Mail.SendToSingleRecipientAsync(to, from, subject, htmlContent, textContent);

There are also other methods to send emails when you need more fine grained control. for example, if you need to use mail merge personalization or you need to set a custom unsubscribe url, etc.

@jduncanator
Copy link

@rakker91 gets across the message I was trying to get across, much better than I did.

We are working with a C# API. I do not want to have to worry about calling REST endpoints, or look up your API documentation online to figure out how to use the library. The library should abstract away all the REST logic behind the scenes. I should be able to figure out how to use the library purely via IntelliSense (something that would be impossible with the current REST implementation of the v3 API, but something that was possible via the old v2 API).

You've made good progress so far, now getting rid of the "REST Client" is the last hurdle I see before we can start merging it into our code and provide much better feedback on issues.

@thinkingserious
Copy link
Contributor Author

Hello everyone,

BIG thanks to all of those that have contributed on this thread, we greatly appreciate your patience and support!

The v9beta branch has been pushed to Nuget as a prerelease for testing. Please give it a spin and let us know what you think :)

The main updates are:

  • Removed dynamic dependencies
  • Added support for .NET Standard 1.3
  • Updated the Mail Helper for better UX/DX

Here is what we think needs to be completed before we exit beta:

  • A few weeks of iterative improvements based on your feedback and internal testing
  • Updated docs (USAGE, USE_CASES, TROUBLESHOOTING, CONTRIBUTING, /examples and the Helper Example)
  • Update the auto-generated portions of the tests to use CamelCase function names
  • Move from Travis to Appveyor
  • Implement SendSingleEmailToMultipleRecipientsAsync
  • Implement SendMultipleEmailsToMultipleRecipientsAsync
  • Adopt StyleCop

The remainder of our wants for this version will be added to GitHub as issues.

With Best Regards,

Elmer

@jduncanator
Copy link

It's looking heaps better so far (from the few minutes I've taken looking at it). I'll find some time this week to look over the new API.

Thanks for all the work put into this.

@thinkingserious
Copy link
Contributor Author

Thanks @jduncanator!

I'm looking forward to everyone's feedback and launching this release in the stable channel on Nuget.

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