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

Support ASP.NET Core 3.0 #280

Merged

Conversation

@martincostello
Copy link
Member

commented Mar 6, 2019

Adds support for ASP.NET Core 3.0 to various simple all providers.

So far, this PR has only updated providers that had one explicit usage of Newtonsoft.Json. All others have been left targeting .NET Core 2.0 for now.

There's lots of #if and copied code here - I'm sure there's a better way it could be factored.

Relates to #275.

@PinpointTownes

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Thanks for your PR!

It looks good but let's just get rid of the netstandard20 TFM: ASP.NET Core 3.0 will be .NET Core-only so there's no point trying to support .NET Standard (and having 2 TFM that target different versions is a terrible recipe for a disaster 😅)

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Great, that’s much easier. I assumed you’d want to keep both so fixed could be back-ported to 2.0 users, but if that’s not a problem than that makes this much easier 😃

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

This should be mostly functional for ASP.NET Core 3.0 now, based on preview 3. I can't test all the providers, but most are simple migrations based on aspnet/AspNetCore#7105.

However, there's four bits that need further review:

  1. Adding the email to the payload for VKontakte:
    // TODO Work out the best way to achieve this with System.Text.Json
    ////payload.Add("email", tokenEmail);
  2. Determine the best way to copy the tokens for Yammer:
    // HACK Work out the best way to do this with System.Text.Json
    using (var stream = new MemoryStream())
    {
    WriteAccessToken(accessToken, stream);
    var copy = JsonDocument.Parse(stream);
    return OAuthTokenResponse.Success(copy);
    }
  3. Determine the best way to copy the tokens for StackExchange:
    // HACK Work out the best way to do this with System.Text.Json
    using (var stream = new MemoryStream())
    {
    CopyPayload(content, stream);
    var copy = JsonDocument.Parse(stream);
    return OAuthTokenResponse.Success(copy);
    }
  4. Determine the best way to copy the tokens for QQ:
    // HACK Work out the best way to do this with System.Text.Json
    using (var stream = new MemoryStream())
    {
    CopyPayload(content, stream);
    var payload = JsonDocument.Parse(stream);
    return OAuthTokenResponse.Success(payload);
    }
@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 9, 2019

I've tested the updated Amazon authentication handler with the 3.0 branch of my site (martincostello/alexa-london-travel-site#255), and things are working as expected.

@niltor

This comment has been minimized.

Copy link

commented Mar 13, 2019

When it can be updated to nuget?

@PinpointTownes

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

However, there's four bits that need further review.

/cc @Tratcher @ahsonkhan could you please shed some light on this?

When it can be updated to nuget?

We'll likely make them available via the MyGet.org feed but I wouldn't expect a public release (i.e on NuGet.org) before 3.0 general availability.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@PinpointTownes FYI I’m working on a separate PR that I’ll submit for separate review at the weekend to add some integration tests for a couple of the providers as a proof-of-concept. If that’s acceptable when I submit it I’ll add it to this PR so we’re confident the changes are all working as I can only easily test the Amazon one myself.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

Draft tests PR for review opened in #282.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

Excluding any code improvements that could be made as referenced by my earlier #280 (comment), I believe this PR is now feature complete.

@martincostello martincostello changed the title [WIP] Support ASP.NET Core 3.0 Support ASP.NET Core 3.0 Mar 17, 2019

@martincostello martincostello marked this pull request as ready for review Mar 17, 2019

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2019

Thanks for the review @Tratcher @bartonjs, I’ll push up some changes tomorrow.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

@Tratcher I've updated the hacky "copy" code to use some of the new preview 5 APIs in 9a80d10. If you get some time, could you take a quick look to check that the approach is a valid usage of the new bits?

@Tratcher

This comment has been minimized.

Copy link

commented May 9, 2019

Looks cleaner. @BrennanConroy?

@PinpointTownes

This comment has been minimized.

Copy link
Member

commented May 9, 2019

@Tratcher are there plans to eventually add less hackish APIs to support creating JSON constructs without a backing array (a-la JObject)?

I hate to be that guy, but I'm sure that System.Xml.Linq (and even System.Xml 😅) currently offer a better experience for these scenarios.

@BrennanConroy

This comment has been minimized.

Copy link

commented May 9, 2019

Yeah looks good.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

@Tratcher @BrennanConroy Thanks both.

@martincostello martincostello added this to the 3.0.0 milestone May 9, 2019

@Tratcher

This comment has been minimized.

Copy link

commented May 9, 2019

@PinpointTownes I completely agree and have given the Json folks that feedback.

The other possibility here is that this isn't the right datatype to expose in the API, we should have used something more abstract that you could implement using a JsonDocument or your own underlying structure. I hesitate to invent a new abstraction though, and I wouldn't want to use an existing one like nested IDictionary<string, object>. Open to suggestions.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented May 29, 2019

Need to fix this either now or when preview 6 arrives: aspnet/AspNetCore#10615 (comment)

@martincostello martincostello force-pushed the martincostello:AspNetCore-300 branch from 7584900 to d294679 Jun 1, 2019

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Jun 6, 2019

@PinpointTownes This PR is getting quite large and unwieldy now, and there's still at least 2 more previews to go until RTM. I was thinking about creating a dev/3.0.0 branch off master, re-targeting this PR to it and then merging.

Then I can do PRs for each preview into that branch until 3.0 ships, and then do a final squash PR back to dev when the time comes. The added benefit of that approach is that 3.0 preview supporting packages get published to MyGet.

Thoughts?

@PinpointTownes

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Do you still expect major changes in the 2.x packages? If not, we should probably simply merge this PR into dev and forget about 2.x stuff. If we need to release patches, we can simply use the 2.1.0 tag and create a new patch branch from there.

@martincostello

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

I'm bit wary about making the main dev branch be against a preview version, rather than a stable one.

Otherwise it might discourage the community from submitting new providers as they'd have to work against preview API surfaces they might not know as much about (e.g. the new JSON stuff) for the next 4 months until 3.0 RTM lands.

@martincostello martincostello force-pushed the martincostello:AspNetCore-300 branch from d294679 to e122c92 Jun 13, 2019

@martincostello martincostello force-pushed the martincostello:AspNetCore-300 branch from ffd2a2b to 771ba60 Jun 13, 2019

@martincostello martincostello self-assigned this Jun 22, 2019

martincostello added some commits Jun 22, 2019

Update providers to ASP.NET Core 3.0
Update all providers to ASP.NET Core 3.0, using preview 6.
Remove KoreBuild
Remove usage of KoreBuild as it is obsolete and no longer supports .NET Core 3.0 after preview 5.
Add code coverage to test project.

@martincostello martincostello force-pushed the martincostello:AspNetCore-300 branch from 49dd868 to 16f2121 Jun 22, 2019

@martincostello martincostello changed the base branch from dev to dev-3.0.0 Jun 22, 2019

@martincostello martincostello merged commit f92639d into aspnet-contrib:dev-3.0.0 Jun 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@martincostello martincostello deleted the martincostello:AspNetCore-300 branch Jun 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.