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

Update sample to use ASP.NET Core 2.1 and HttpClientFactory #23

Merged
merged 25 commits into from May 30, 2018

Conversation

Projects
None yet
3 participants
@martincostello
Copy link
Member

martincostello commented Mar 11, 2018

This Pull Request updates the sample application to use ASP.NET 2.1 and HttpClientFactory (initially with preview 1, with the intention being to move to the final 2.1.0 version before merging).

The sample now uses Refit to show usage with typed clients, rather than using HttpClient directly in the controller. This also makes the code a bit cleaner as the GitHub specific configuration (like setting the User-Agent header) is abstracted away.

Some ASP.NET Core 2.1 idioms as described in the announcement have also been added.

The processing of plugging this in has raised a few questions about HttpClientFactory itself, which I'll raise issues to ask in their repo. At a high-level these are:

  1. Adding overloads to some extension methods to allow IServiceProvider to be accessed while configuring clients (aspnet/HttpClientFactory#75). ✔️
  2. Is there a cleaner way to set the order of delegating handlers? Use IHttpMessageHandlerBuilderFilter.

As well as the final upgrade before merge, requires updates to the README in the repo root as well.

Relates to #6.

martincostello added some commits Mar 11, 2018

Tidy-up launchSettings.json
Tidy-up launchSettings.json to remove Intellisense warnings.
Update samples for ASP.NET Core 2.1
Update the sample application to use ASP.NET Core 2.1, HttpClientFactory and Refit for #6.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 11, 2018

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #23   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files           8        8           
  Lines         720      720           
  Branches       96       96           
=======================================
  Hits          719      719           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1213f04...0171a4a. Read the comment docs.

martincostello added some commits Mar 11, 2018

Update samples' README
Update the README for the samples for ASP.NET Core 2.1 and add it to the solution file.
Simplify registration
Simplify registration of IGitHub by using another overload of AddTypedClient().
Simplify handler registration
Simplify handler registration for ensuring that the intercepting handler is placed in the pipeline after the application's handlers.
@martincostello

This comment has been minimized.

Copy link
Member

martincostello commented Mar 11, 2018

Regarding question 2 in the description, this was caused by the handler registration in the test project running before the applications. Once I realised this, it was easy to refactor the code slightly to ensure the application always put its handlers at the top of the list of additional handlers.

Simplify sample further
Further simplify the samples by using IHttpMessageHandlerBuilderFilter to configure the intercepting handler last.
@martincostello

This comment has been minimized.

Copy link
Member

martincostello commented Mar 11, 2018

Digging around in the code I found the IHttpMessageHandlerBuilderFilter extension point, which makes things even simpler.

martincostello added some commits Mar 11, 2018

Update sample README
Update the samples' README to reflect the changes in the previous commit.
Fix SDK version
Add the build number to the end of the SDK version for preview 1.
Run sample tests in CI
Run the tests for the sample application as part of the CI build.
Remove Microsoft.VisualStudio.Web.CodeGeneration.Tools
Remove Microsoft.VisualStudio.Web.CodeGeneration.Tools as it breaks the build (and might be redundant what with global tools anyway).
Update .NET test SDK
Update the .NET Core test SDK.
Use Microsoft.AspNetCore.Mvc.Testing for sample tests
Use the new Microsoft.AspNetCore.Mvc.Testing NuGet package to simplify the tests in the sample application.
Update to .NET Core 2.1. Preview 2
Update to preview 2 of .NET Core 2.1.
@@ -13,7 +13,7 @@
<ProjectReference Include="..\..\src\HttpClientInterception\JustEat.HttpClientInterception.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.6.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.6.1" />

This comment has been minimized.

@martincostello

martincostello Apr 13, 2018

Member

Need to update to 15.7.0.

@@ -13,7 +13,7 @@
<ProjectReference Include="..\..\src\HttpClientInterception\JustEat.HttpClientInterception.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.6.0" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.6.1" />
<PackageReference Include="Moq" Version="4.8.2" />
<PackageReference Include="Newtonsoft.Json" Version="11.0.1" />

This comment has been minimized.

@martincostello

martincostello Apr 13, 2018

Member

Update to 11.0.2.

martincostello added some commits Apr 27, 2018

Remove relative path to ruleset
Use $(MSBuildThisFileDirectory) to reference the ruleset file, instead of a relative path.
Update test dependencies
Update Newtonsoft.Json, the .NET Core test SDK and BenchmarkDotNet to their latest versions.

martincostello added some commits Apr 27, 2018

Target .NET Core 2.1 in benchmarks project
Update the benchmarks project's target framework to netcoreapp2.1.
Add memory diagnoser to benchmarks
Configure the memory diagnoser on the benchmarks to collect metrics about allocations.
Update to .NET Core 2.1 RC1
Update to RC1 of .NET Core 2.1.
Workaround missing GitHub release
Work around the SDK download script not being found due to RC1 CLI not having a relaese in GitHub like the previews did.

martincostello added some commits May 26, 2018

Use final ASP.NET Core 2.1 release
Use the early access NuGet feed to migrate to the RTM version of ASP.NET Core 2.1.
Upgrade the .NET Core test SDK and Refit to their latest versions.
Fix two code analysis warnings.

@martincostello martincostello modified the milestones: Future, vNext May 26, 2018

martincostello added some commits May 30, 2018

Remove early-access download NuGet feed
Remove the early-access download feed URL for 2.1.0 now that the packages have been published to nuget.org.
Use tag to download obtain script
Revert to using tags to get the script to download the SDK.

@martincostello martincostello changed the title [WIP] Update sample to use ASP.NET Core 2.1 and HttpClientFactory Update sample to use ASP.NET Core 2.1 and HttpClientFactory May 30, 2018

@martincostello martincostello merged commit 9f89324 into justeat:master May 30, 2018

4 checks passed

codecov/patch Coverage not affected when comparing 1213f04...0171a4a
Details
codecov/project 99.86% remains the same compared to 1213f04
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@martincostello martincostello deleted the martincostello:aspnet-core-210 branch May 30, 2018

@slang25
Copy link
Contributor

slang25 left a comment

This is great work! Congrats

@martincostello martincostello referenced this pull request Jul 17, 2018

Closed

Add Source Link #16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment