Skip to content

Support ASP.NET Core 3.0 #280

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
Merged
merged 28 commits into from
Nov 6, 2019
Merged

Support ASP.NET Core 3.0 #280

merged 28 commits into from
Nov 6, 2019

Conversation

benmccallum
Copy link
Contributor

Addresses #276

@benmccallum
Copy link
Contributor Author

benmccallum commented Oct 18, 2019 •
edited
Loading

I think this should do it, but @EdgarMaucourant did mention he had issues in GraphQL proper over on the issue, so potentially we need to do a similar thing over in that project.

Only thing I needed to toggle with #if blocks was the .AddAuthorization which was renamed to .AddAuthorizationCore to address this breaking change.

My biggest unknown about all this is can an ASP.NET Core 3.0 app be referencing projects that target netstandard2.x and reference packages like Microsoft.Extensions.Xyz at v2.x.x? I've got no idea what would happen. I assume the ASP.NET Core 3.0 app will get the shared framework, which potentially includes v3.0.0 of those, so it would have to be able to fwd and be compatible. This really isn't my area of expertise.

@benmccallum
Copy link
Contributor Author

Few more things to sort out, but getting there slowly...

@benmccallum
Copy link
Contributor Author

benmccallum commented Oct 18, 2019 •
edited
Loading

So I've got all tests passing except websocket ones on 3.0.

Outstanding is:

  1. WebSocketConnectionFacts tests are failing under netcoreapp3.0, this is above my head, perhaps @pekkah would know a solution :P Getting a System.InvalidOperationException : Incomplete handshake, status code: 404 which could be a breaking change in 3.0 shrugs
  2. Cake build file updates, would love if someone could jump in here for that too. Potentially that involves AppVeyor configuration as well depending on where the SDK/s for builds are configured. Haven't really looked into it yet.
  3. Find out if this even works on GraphQL proper.

sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r reviewed Oct 18, 2019
View reviewed changes
sungam3r
sungam3r requested changes Oct 18, 2019
View reviewed changes
Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

In general, the direction is right.

@benmccallum
Copy link
Contributor Author

benmccallum commented Oct 25, 2019 •
edited
Loading

Hey @sungam3r, ready for another review.

  • I've resolved all the ones that I agreed with and changed.
  • I've commented on the references ones that have to be like that - linked some sources. Resolve these when you can.
  • Outstanding is:
    • Linked and compiler toggled Program and Startup? Or keep separate?
    • Go back to TestServer or leave as standard/documented approach?
    • Someone to help me fix the WebSockets tests.
    • Someone to help me fix the build on AppVeyor for 3.0 target

@sungam3r
Copy link
Member

  1. Linked and compiler toggled Program and Startup? Or keep separate?
  2. Go back to TestServer or leave as standard/documented approach?
  3. Someone to help me fix the WebSockets tests.
  4. Someone to help me fix the build on AppVeyor for 3.0 target
  1. I am for linked.
  2. I am for TestServer. And isn't it documented?
  3. I can’t do it yet.
  4. I guessed that it would have to be done. I am not familiar with appveyor but I think that you should start with changing 2017 to 2019 and 2.2.104 to 3.0.100 in config

@benmccallum
Copy link
Contributor Author

benmccallum commented Nov 5, 2019 •
edited
Loading

Found the AppVeyor issue. Fresh eyes. This was the fix. bdd139b

I realised that the thing it was complaining about "GraphQL.Samples.Server doesn't support netcoreapp2.0, it's netcoreapp3.0 only" or whatever, was specifically referring to the assembly name in the .csproj, as the project name doesn't include GraphQL.. So then I found that I hadn't updated the assembly name for the new 3.0 project and so that messes up the test run. Kinda weird it worked locally though...

So the last thing here is the WebSockets unit tests still failing. I'm really not across this stuff but I'll have a quick look. @pekkah, sorry to bother, but you seem good at this, mind taking a quick look at it?

@benmccallum
Copy link
Contributor Author

Latest commit gets rid of the 3.0 specific samples server project all together, since I realised all the files were linked anyway...

@benmccallum
feat: Don't use 2.2.x version of websockets pkg in 3.0
031f123
@benmccallum
refactor:
19fe3c3 
Remove TestHost package reference as it's covered by Mvc.Testing one.
Bump some nuget versions for test deps.
Remove static file middleware from samples server as it's unnecessary.
@sungam3r
Copy link
Member

sungam3r commented Nov 5, 2019

Latest commit gets rid of the 3.0 specific samples server project all together, since I realised all the files were linked anyway...

It is interesting to observe such an evolution. I was hoping that everything would come down to one sample project with multitargeting. It seems to me that this is better than individual examples. It’s easier to focus and understand the difference between the platforms.

@benmccallum benmccallum mentioned this pull request Nov 5, 2019
Closed
@benmccallum
Copy link
Contributor Author

Boom, got it.

Bit painful, but looks like if you call .Configure(() => {}) on a IWebHostBuilder instance it'll take precedence over the Configure method in TStartup when calling .UseStartup<TStartup>(). Middleware wasn't being hooked up, hence the 404 behaviour and the middleware not firing.

What's next @sungam3r, is this ready for prime time? Did a bit more clean up as I went through all this at least :P

@benmccallum benmccallum marked this pull request as ready for review November 5, 2019 10:57
@benmccallum benmccallum requested a review from sungam3r November 5, 2019 11:00
@sungam3r
Copy link
Member

sungam3r commented Nov 5, 2019

Bit painful, but looks like if you call .Configure(() => {}) on a IWebHostBuilder instance it'll take precedence over the Configure method in TStartup when calling .UseStartup().

It's good that everything ended successfully. I will look again later. I don’t think I will have any new comments.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
samples/Samples.Server/Program.cs Outdated
@@ -1,5 +1,6 @@
using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Hosting;
using Microsoft.Extensions.Hosting;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe take using Microsoft.AspNetCore.Hosting; and using Microsoft.Extensions.Hosting; under #if / #else too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I wasn't sure what was best practice with that. Will #if/#else them to make it clearer.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
samples/Samples.Server/Program.cs Outdated
public static IWebHostBuilder CreateWebHostBuilder(string[] args)
{
return WebHost.CreateDefaultBuilder<Startup>(args)
.UseSerilog();
}
#else
public static IHostBuilder CreateHostBuilder(string[] args)
Copy link
Member

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting Ctrl, K + F, only works on current target code. Fixed.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
tests/Samples.Server.Tests/BaseTest.cs Outdated
using System.Net.Http;
using System.Text;
using System.Threading.Tasks;
using Microsoft.Extensions.Hosting;
Copy link
Member

Choose a reason for hiding this comment

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

under #if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
tests/Samples.Server.Tests/Samples.Server.Tests.csproj Outdated
<PackageReference Include="Microsoft.AspNetCore.App" />
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="$(MicrosoftAspNetCoreTestHostVersion)" />
<PackageReference Include="GraphQL" Version="$(GraphQLVersion)" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="$(MicrosoftAspNetCoreMvcTestingVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Microsoft.AspNetCore.TestHost should be enough. There are no any MVC things in this repo. Also no need in MicrosoftAspNetCoreMvcTestingVersion in Directory.Build.props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
...s/Transports.Subscriptions.WebSockets.Tests/Transports.Subscriptions.WebSockets.Tests.csproj Outdated
<PackageReference Include="Microsoft.AspNetCore" Version="$(MicrosoftAspNetCoreVersion)" />
<PackageReference Include="Microsoft.AspNetCore.TestHost" Version="$(MicrosoftAspNetCoreTestHostVersion)" />
<PackageReference Include="Microsoft.AspNetCore.WebSockets" Version="$(MicrosoftAspNetCoreWebSocketsVersion)" />
<PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="$(MicrosoftAspNetCoreMvcTestingVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

TestHost instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
tests/Transports.Subscriptions.WebSockets.Tests/WebSocketsConnectionFacts.cs Outdated
{
_server.Dispose();
#if !(NETFRAMEWORK || NETCOREAPP2_2)
_host.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

sungam3r
sungam3r reviewed Nov 5, 2019
View reviewed changes
tests/Samples.Server.Tests/BaseTest.cs Outdated
}

protected TestServer Server { get; }

protected HttpClient Client { get; }

#if !NETCOREAPP2_2
protected IHost Host { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Protected? Any usages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, neither for the two above. All private now.

sungam3r
sungam3r requested changes Nov 5, 2019
View reviewed changes
Copy link
Member

@sungam3r sungam3r left a comment

Choose a reason for hiding this comment

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

LGTM
I will be glad to merge this after the final corrections.
It was a good experience migrating to netcoreapp3.0 while maintaining compatibility with the previous platform.

@benmccallum
Copy link
Contributor Author

@sungam3r , all feedback addressed. LGTM

@sungam3r sungam3r added the enhancement New feature or request label Nov 6, 2019
@sungam3r sungam3r merged commit ff4e7e7 into graphql-dotnet:develop Nov 6, 2019
@sungam3r
Copy link
Member

sungam3r commented Nov 6, 2019

Finally!

rose-a pushed a commit to rose-a/server that referenced this pull request Mar 23, 2020
@benmccallum @rose-a
Support ASP.NET Core 3.0 (graphql-dotnet#280)
ff48406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sungam3r sungam3r sungam3r approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benmccallum @sungam3r @pekkah