Jericho / StrongGrid

Strongly typed library for the entire SendGrid v3 API, including webhooks
187 stars 38 forks source link

Check attachment size #191

Closed Jericho closed 7 years ago

Jericho commented 7 years ago

SendGrid doesn't specifically restrict the size of a given attachment but they limit the "total" size of an email to 30MB. Therefore, it makes sense to limit the size of a given attachment to 30MB.

During a discussion I had with @shortstuffsushi, he made me realize that StrongGrid currently allows developers to attach files up to 2GB (attaching such a large file would uses a lot of memory and would take quite a while) and we know that it would eventually be rejected by SendGrid as being excessively large. That's why I think it makes sense to limit the size of an individual attachment.

shortstuffsushi commented 7 years ago

Hi @Jericho,

I decided to pull this comment out of the other issue as it didn't really pertain to the pull request. I guess I'm not sure if it does here, but it seemed like something that could be brought up since this is an issue / discussion thread.

I'm not really familiar with your library or the SendGrid proper packages, since I usually just send directly via an SmtpClient. Is there a reason you're replicating the efforts of this repository in your own? It seems yours stems out of differences that have since been resolved (the usage of dynamic primarily). Since this one mentions that it's hoping to be community driven, why not merge your effort in here. It seems clear you've put a lot of work into that one already, why keep two separate efforts going?

Jericho commented 7 years ago

I explain in my readme that I originally forked SendGrid's repo and submitted a PR to add strong typing and netstandard support to their library but they rejected my submission. I even had a phone conversation with their product manager but at the time, they were convinced their "dynamic" approach was the right way. That's what originally prompted me to create the StrongGrid library.

Since then (probably due to community pressure), they have changed their stance and they proceeded to remove all "dynamic" types from their library and to support netstandard as well but to this day they only support the narrow use case of sending transactional emails. Granted, that's the most popular use case but it's not the only one. Their API supports other "email marketing" use cases such as: maintaining lists and segments, importing contacts, sending bulk emails (AKA campaigns), getting stats for previously sent campaigns, etc. Their library doesn't support any of these scenarios which StrongGrid happily supports.

Also, SendGrid can be configured to send you a WebHook when an inbound email is received or when a email event occurs (such as open, click, bounce, etc.). The data posted by SendGrid in the WebHook needs to be parsed but they don't provide any way to conveniently parse this information and consume it in .NET. StrongGrid provides a parser for their WebHooks that returns strongly typed objects.

So, that's why I think StrongGrid is still relevant.

shortstuffsushi commented 7 years ago

Your original two points for forking are valid -- dynamic was gross hack. I'm glad they came around to that, although it's unfortunate that your work didn't make it in. From the PR referenced in the readme, rather than rejecting, it appears they instead basically rewrote their lib and couldn't merge your changes in. I understand not wanting to make the same changes twice, though, that'd be a pain.

Anyway, at this point, the bonus features StrongGrid add still seem like a great candidate for SendGrid core. I would gladly use them, but to be honest, am unlikely to pick up a non-sanctioned lib to do so, and I feel like this is pretty standard. As a developer, I'd likely search NuGet and the internet for the SendGrid package (to be honest, this is exactly what I did a few weeks ago when I pulled their package in). I searched for their official version, and entirely ignored anything else for fear that it would either be or become stale due to the maintainer losing interest, or not be as good of quality since it isn't maintained by SendGrid proper. Whether this is the right or wrong approach, it's at least the way I typically look at pulling in packages.

Is there any way you could be talked into pulling these additional features into their core offering at this point? To be honest, if you had a check list of things to be ported over, I'd be glad to help.

Jericho commented 7 years ago

I completely understand your stance regarding only using "official" libraries but there are many more people who don't have the same stance and will use the library which they feel is "best". I see the number of downloads go up and the number of people who engage me in discussion, report issues, etc. to lead me to believe that there is a "market" for non-official libraries.

If SendGrid expressed interest in integrating StrongGrid into their library, I would talk to them about it. But so far they haven't expressed such interest. If you have influence with them, go ahead and share your idea with them and let's see.

As far as a check list, I can tell you that StrongGrid supports 30 of their API resources (email being one of them) each one comprised of many methods such as Get, GetAll, Create, Update, etc. So we would need to port 29 of them in addition to 2 webhook parsers.

shortstuffsushi commented 7 years ago

To your first paragraph -- why not both the best and the official at the same time? It seems unlikely that they'll reach out to you to do so, but their repo indicates interest in community development. I have no ties with them to try to influence that one way or the other, my interest is solely for my own use of the library (well, my own use at my company, but not on behalf of SendGrid). I'll take another spin through you're repo to see if I could come up with an actual list and then perhaps make an issue that can track the completion of merging the two libraries if you're interested. I think it'd be for the best for both libraries (especially as things like my "size requirements" come up, no need to duplicate the work). Are you interested in that?

thinkingserious commented 7 years ago

tl;dr; Integrating StrongGrid into the "official" library would be a MASSIVE win for the SendGrid community. Please let me know how I can help.

@Jericho, @shortstuffsushi,

The reason I thought dynamic was the right thing at that time, was that I was thinking in terms of how can I update all 7 of our SDKs the fastest way possible (perhaps biting off more than I could chew at the time), then iterate based on feedback from the respective communities. At the time, I lumped C# in the dynamic pile along with PHP, Python and Ruby, which was a horrible mistake and very lame. That said, before I began that work I asked many developers, including the SendGrid C# community, for feedback and I received nothing but thumbs up at the time and I mistakenly took that as a global thumbs up from the community. Lesson learned the hard way. But out of all that process and huge mistake, the most awesome thing happened when I pushed up the changes. Excellent feedback came out the woodworks and we were able to correct our course as planned.

@Jericho

You have been an incredible asset to the SendGrid C# community and I hope you will continue to do so. I have no problem granting you tons of credit as a core library author if you decide to merge your work back into the "official" library. Note that it was never a personal thing regarding merging your changes, I have great respect for you and your work. It just would have been really hard at the time for me to merge them (at least it felt so at the time) and probably would have thrown my short-sighted timeline out of whack.

@shortstuffsushi,

Thank you very much for pushing this conversation towards a very positive outcome!

BTW, you both might be interested in this ;) https://sendgrid.com/blog/announcing-hacktoberfest-2017/

Jericho commented 7 years ago

@thinkingserious you and @mbernier want to have a Skype call to discuss?

mbernier commented 7 years ago

@Jericho Got time Friday, 11MT to 1:30MT? I have your email, so if it works I can send out an invite :)

@shortstuffsushi wanna join? (my email is just my hipchat name at sendgrid.com)

Jericho commented 7 years ago

@mbernier Friday 11MT is good with me

shortstuffsushi commented 7 years ago

Hi guys. Sorry for the delay, I don't get to open source much during the week. I'm interested in helping out where help is needed, but I'm not sure if I'll be helpful on the call since I'm pretty new to both of these projects. Let me know if you specifically need me for something, otherwise I'm more likely to just jump in on th implementation of the migration. Interested in helping out for Hacktoberfest for sure though. Will keep an eye on it.

thinkingserious commented 7 years ago

@Jericho,

Tomorrow am PST and Friday afternoon PST works for me, but neither of those time slots work for Matt :(

In the interest of moving forward faster, could we talk tomorrow morning, say 10am PST, then follow up with @mbernier afterwards? I am on Skype.

With Best Regards,

Elmer

Jericho commented 7 years ago

@thinkingserious sure, let's talk tomorrow 10am PST.

thinkingserious commented 7 years ago

Awesome! Thanks for the flexibility :)

mbernier commented 7 years ago

Cool, thanks, fellas. I am excited about the possibilities of what you guys come up with!

shortstuffsushi commented 7 years ago

Hi guys. I kinda fell off the map on this one. Did anything come out of your meeting that I could help with?

Jericho commented 7 years ago

See survey: https://github.com/sendgrid/sendgrid-csharp/issues/530

Option 1 is for you! ;-)