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.

Sign up for GitHub

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

Jump to bottom

feat: Integrate SendGrid with HttpClientFactory #922

Merged
merged 9 commits into from
May 28, 2020
Merged

feat: Integrate SendGrid with HttpClientFactory #922

merged 9 commits into from
May 28, 2020

Conversation

akunzai
Copy link
Contributor

@akunzai akunzai commented Aug 19, 2019 •
edited
Loading

Fixes #880

Checklist

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

  • Introduce new SendGrid.Extensions.DependencyInjection package for HttpClientFactory integrations

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Aug 19, 2019
aevitas
aevitas suggested changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@aevitas aevitas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from some very minor things. This is definitely the way to go if we want to retrofit ASP.NET Core DI support without burdening the users who don't want to use it with all of its dependencies.

tests/SendGrid.Extensions.DependencyInjection.Tests/ServiceCollectionExtensionsTests.cs Outdated Show resolved Hide resolved
src/SendGrid.Extensions.DependencyInjection/InjectableSendGridClient.cs Outdated Show resolved Hide resolved
@aevitas
Copy link
Contributor

aevitas commented Aug 19, 2019

LGTM 👍

@akunzai akunzai changed the title integrate SendGrid with HttpClientFactory WIP: Integrate SendGrid with HttpClientFactory Aug 19, 2019
@akunzai akunzai changed the title WIP: Integrate SendGrid with HttpClientFactory Integrate SendGrid with HttpClientFactory Aug 19, 2019
@akunzai
Copy link
Contributor Author

akunzai commented Aug 19, 2019 •
edited
Loading

Hi @thinkingserious,

After some investigations on Travis CI,

this PR include some building optimizations.

the building/testing time is reduced from 10 minutes to 3 minutes now.

@akunzai
Copy link
Contributor Author

akunzai commented Aug 20, 2019 •
edited
Loading

Hello @aevitas @thinkingserious,

Please let me know if you have any suggestions.

aevitas
aevitas reviewed Aug 24, 2019
View reviewed changes
README.md Show resolved Hide resolved
@aevitas
Copy link
Contributor

aevitas commented Aug 24, 2019

@akunzai Great job on the optimizations and upgrading the projects from netcore 1.0. I don't think anyone can be assumed to be using those runtimes at this point anymore.

I've just left some very minor comments on the readme concerning accessors. I think it would make sense to keep the code uniform in terms of accessors, but it's up to you.

@IanKemp
Copy link

IanKemp commented Sep 5, 2019

Sooo... is this likely to get merged anytime soon, since everyone appears to be happy with it?

@IanKemp IanKemp mentioned this pull request Sep 5, 2019
Closed
7 tasks
@aevitas
Copy link
Contributor

aevitas commented Sep 5, 2019

@IanKemp Depends on when @thinkingserious gets around to it.

@akunzai
Copy link
Contributor Author

akunzai commented Sep 15, 2019

latest commit fixes:

  • Floating StyleCop.Analyzers version
  • Add StyleCop.Analyzers to SendGrid.Extensions.DependencyInjection
  • Show StyleCop.Analyzers warnings in travis

@ryanjerskine
Copy link

Any updates on this? Seems like a valuable PR that will prevent me from rewriting much of the DI myself.

@akunzai
Copy link
Contributor Author

akunzai commented Jan 22, 2020

Hi @thinkingserious,

Conflicts solved for #969.

Automated deploy of the SendGrid.Extensions.DependencyInjection.*.nupkg was ignored.

Please let me know if you prefer automated deploy the SendGrid.Extensions.DependencyInjection.*.nupkg too.

@ComtelJeremy
Copy link

@thinkingserious Any progress on this? We would like to use it in our app, since we are using SendGrid with an Asp.Net Core web app. Or is there a different solution in the works to use HttpClientFactory?

@hamakiri
Copy link

hamakiri commented Apr 10, 2020 •
edited
Loading

Will this be merged at some point ?
Really would like to use that feature without creating my own "InjectableSendGridClient".
Thanks.

@alex-zwingli
Copy link

@thinkingserious Any updates on this PR?

@akunzai
Copy link
Contributor Author

akunzai commented May 17, 2020

Hi @thinkingserious,

some conflicts was solved,

And the latest commit will automated deploy the SendGrid.Extensions.DependencyInjection.*.nupkg.

Are you prefer SendGrid.Extensions.DependencyInjection version number begin from 1.0.0 or it should same as SendGrid's version number?

eshanholtz
eshanholtz reviewed May 26, 2020
View reviewed changes
.env_sample Outdated Show resolved Hide resolved
eshanholtz
eshanholtz reviewed May 26, 2020
View reviewed changes
README.md Show resolved Hide resolved
eshanholtz
eshanholtz reviewed May 26, 2020
View reviewed changes
README.md Show resolved Hide resolved
eshanholtz
eshanholtz reviewed May 26, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
@aevitas
Copy link
Contributor

aevitas commented May 26, 2020

Someone ship this man a medal after this gets merged.

@akunzai
Integrate SendGrid with HttpClientFactory
de66186 
- Introduce new SendGrid.Extensions.DependencyInjection package for HttpClientFactory integrations
@akunzai
Update SendGrid.Tests
5bc52b2 
- Update NuGet packages
- Cleanup Properties
eshanholtz
eshanholtz approved these changes May 28, 2020
View reviewed changes
Copy link
Contributor

@eshanholtz eshanholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏅

@eshanholtz eshanholtz merged commit cadb619 into sendgrid:master May 28, 2020
@eshanholtz eshanholtz changed the title Integrate SendGrid with HttpClientFactory feat: Integrate SendGrid with HttpClientFactory May 28, 2020
@akunzai akunzai deleted the integrate-http-client-factory branch May 28, 2020 21:36
@ComtelJeremy
Copy link

Thank you for this, we will put it to use right away. Just wondering when the nuget for SendGrid.Extensions.DependencyInjection will be published. Thank you, again.

@eshanholtz
Copy link
Contributor

Next release is scheduled for 2020-06-10

@arielmoraes
Copy link

arielmoraes commented May 28, 2020 •
edited
Loading

Shouldn't the InjectableSendGridClient constructor accept an IOptionsMonitor instead of IOptions? Asking this because the later one doesn't support reloading.

@akunzai
Copy link
Contributor Author

akunzai commented May 28, 2020 •
edited
Loading

@arielmoraes The SendGridClientOptions seems no need to change frequently, and also this PR was merged. (your should join to review this PR earlier)

If it happens in your scenario, another PR is welcome!

@childish-sambino childish-sambino mentioned this pull request Jun 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aevitas aevitas aevitas requested changes

@IanKemp IanKemp IanKemp left review comments

@eshanholtz eshanholtz eshanholtz approved these changes

@thinkingserious thinkingserious Awaiting requested review from thinkingserious

Assignees
No one assigned
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dependency Injection in Startup.cs
10 participants
@akunzai @aevitas @IanKemp @ryanjerskine @ComtelJeremy @hamakiri @alex-zwingli @eshanholtz @arielmoraes @thinkingserious