aspnet / AspNetWebStack

ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core)
Other
858 stars 354 forks source link

Move NetCore project to use .NET SDK #380

Closed dougbu closed 1 year ago

dougbu commented 1 year ago

Move NetCore project to use .NET SDK

React to using Newtonsoft.Json.Bson everywhere

Remove unnecessary ConcurrentDictionary class

Remove UriQueryUtility because WebUtility is available everywhere

Add simple classes to NetCore project

Remove unnecessary NetCore-specific GlobalSuppressions.cs file

Use NameValueCollection everywhere

Add MaxDepth support in NetCore assembly

Add UseDataContractJsonSerializer support in NetCore assembly

Copy TranscodingStream and related files from dotnet/runtime

Adjust TranscodingStream and related code to compile and run here

Use TranscodingStream in JSON and XML formatters

Add MediaTypeMappings support in NetCore assembly

Add IRequiredMemberSelector support in NetCore assembly

Remove unnecessary NETFX_CORE workarounds

Work around missing features in netstandard1.3

Ensure a couple more assemblies are available

dougbu commented 1 year ago

This PR is pretty big and complicated. I recommend going through the commits individually and concentrating on areas you're at least slightly familiar with.

@phenning, I included you because I'm removing some comments you added. Note we already have a netstandard2.0 assembly w/ none of the restrictions of the former Profile259 assembly.

@stephentoub and @GrabYourPitchforks, you're likely most interested in the first two of the three commits mentioning TranscodingStream, especially the middle one.

dougbu commented 1 year ago

To help track which commits have been thoroughly checked, please tick those you are signing off on:

dougbu commented 1 year ago

/ping reviewers

dougbu commented 1 year ago

Rebased and addressed @stephentoub's comments.

@stephentoub any other thoughts❔ If none, please check the non-NameValueCollection commits off in the earlier checklist that aren't checked yet. They're all TranscodingStream related but I understand if parts aren't of interest to you. We could find someone to double-check the project files for example (if you prefer).

Other reviewers: Please look at the NameValueCollection commit.

stephentoub commented 1 year ago

please check the non-NameValueCollection commits off in the earlier checklist that aren't checked yet. They're all TranscodingStream related but I understand if parts aren't of interest to you. We could find someone to double-check the project files for example (if you prefer).

I don't have rights to click those checkmarks. I only reviewed the "Adjust TranscodingStream and related code to compile and run here" commit and it looked fine other than my feedback you addressed.

dougbu commented 1 year ago

I don't have rights to click those checkmarks.

That's a bit odd but thanks for letting me know where you'd focused. Much appreciated.

dougbu commented 1 year ago

To other reviewers: I also ticked the checkboxes for "Copy TranscodingStream …" because that was just a literal cp from my dotnet/runtime clone and "!fixup! Remove added AggressiveInlining requests" because that was in response to @stephentoub's comments. This leaves us w/ 3 unreviewed commits.

dougbu commented 1 year ago

@Tratcher I addressed your comment on the "Use TranscodingStream …" commit in the latest "!fixup!" commit. Would you say you're signing off on those two commits❔

Still one open question on versioning and public API surface to you @Tratcher

Tratcher commented 1 year ago

I've reviewed everything but the tests. :shipit: