Aspnetcore: Remove Newtonsoft.Json from the shared framework

[natemcmaster]

The following shared-framework libraries pull in Newtonsoft.Json:

• [x] - Microsoft.AspNetCore.Mvc.ViewFeatures
• [x] - Microsoft.Extensions.Configuration.Json
• [x] - Microsoft.Extensions.DependencyModel
• [x] - Microsoft.IdentityModel.JsonWebTokens
• [x] - Microsoft.AspNetCore.SignalR.Protocols.Json
• [x] - Microsoft.AspNetCore.Authentication.OAuth
• [x] - Microsoft.Extensions.Logging.EventSource

These assemblies and dependencies need to be updated to remove their dependency on Newtonsoft.Json.

[natemcmaster]

Tracking external issues:

• https://github.com/dotnet/core-setup/issues/3311
• https://github.com/aspnet/Extensions/issues/568
• https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1060
• https://github.com/aspnet/Extensions/issues/1013

Internal changes needed:

• @Tratcher - update Microsoft.AspNetCore.Authentication.OAuth
• @rynowak @mkArtakMSFT - we need to update Microsoft.AspNetCore.Mvc.ViewFeatures to make sure it doesn't pull in Newtonsoft.Json.
• Update our dependency on IdentityModel.

[natemcmaster]

Also adding @mkArtakMSFT to help with MVC's dependency on Json.net and myself to coordinate with partners on getting Json.net removed.

[natemcmaster]

Pushing out to preview3. We are still blocked on https://github.com/dotnet/core-setup/issues/3311 and https://github.com/aspnet/Extensions/issues/568

[Tratcher]

Partially unblocked by https://github.com/dotnet/corefx/pull/34485

[joshfree]

/cc @ahsonkhan and @steveharter who will have a porting-guide.md (soon) to assist with this

[muratg]

Adding @BrennanConroy for Microsoft.AspNetCore.SignalR.Protocols.Json

[BrennanConroy]

JsonDocument doesn't help us, we need the serializer and deserializer :)

[BrennanConroy]

Also, was there ever a discussion/decision for the netstandard issue?

[muratg]

@joshfree @ahsonkhan @steveharter?

[davidfowl]

Yea there’s a source version of the reader and writer for ns2.0

[ahsonkhan]

Also, was there ever a discussion/decision for the netstandard issue?

Yea there’s a source version of the reader and writer for ns2.0

https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.Bcl.Json.Sources

It contains the JsonDocument as well.

For guidance on consuming the source pacakge, visit: https://aka.ms/json-source-package-guide.

[BrennanConroy]

JsonDocument is now available to our builds.

[natemcmaster]

Looks like DependencyModel has removed Json.Net, Oauth and Config.Json are in the works (https://github.com/aspnet/AspNetCore/pull/7105, https://github.com/aspnet/Extensions/pull/1028) and I believe SignalR is done (https://github.com/aspnet/AspNetCore/pull/6977). I think we'll be able to complete this for Preview 3.

[ahsonkhan]

I believe SignalR is done

I could be wrong, but I think the JsonProtocol is still waiting on serializer/deserializer feature being worked on atm.

[natemcmaster]

Ah, missed that. Ok, so maybe not that close. @BrennanConroy what would be the impact of removing Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson from the shared framework? Are you planning to add a new assembly for a non-JSON.NET implementation of the json protocol?

https://github.com/aspnet/AspNetCore/blob/09e019841c2ff70c14a0ceb2958b1fc4e6bd0c8a/src/Framework/Microsoft.AspNetCore.App.props#L72-L75

[davidfowl]

SignalR not nearly done. A serializer is required.

[BrennanConroy]

Are you planning to add a new assembly for a non-JSON.NET implementation of the json protocol?

Yes.

what would be the impact of removing Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson from the shared framework?

Worst case is we remove the reference in preview3 and require customers to have to add a reference to either the NewtonsoftJson protocol or MessagePack protocol package for signalr to work at all.

[natemcmaster]

Do you have a rough ETA on this? I want to know how bad it would be if we did this for Preview 3. For MVC in Preview 2, we removed the NewtonsoftJson package from the framework. If SignalR is the only thing keeping Json.Net in, I would be tempted to remove it even it means users have to add a new package.

Not committing to this...just thinking out loud.

[BrennanConroy]

For MVC in Preview 2, we removed the NewtonsoftJson package from the framework.

Yeah but it was easier for them because MVC continues to work without the package. Whereas SignalR just doesn't function anymore.

Do you have a rough ETA on this?

We can remove the reference whenever we want, so I suggest we wait until near the end of preview3 to see if we can get the new protocol in.

[muratg]

I'd also be fine to ask users to add the dependency directly for Preview3. cc @bradygaster in case he has any objections.

[davidfowl]

We just need a SignalR template 😬

[bradygaster]

@davidfowl i adore this idea. What would you want to say in said template? I'd prefer to have a template and a scaffolder so we could add it in after-the-fact, too.

[bradygaster]

Worst case is we remove the reference in preview3 and require customers to have to add a reference to either the NewtonsoftJson protocol or MessagePack protocol package for signalr to work at all.

This seems a little painful for customers but if we message it broadly maybe it wouldn't sting so much. want me to start a twitter poll to see if it'd freak folks out? If we can avoid this I'd prefer not randomize customers?

w.r.t. @davidfowl and my aspiration for templates - would it behoove us to augment the existing templates to add a "add signalr?" option? That, when enabled, would lay down everything a customer needs?

@BrennanConroy is it realistic that we could fix this by the end of p3 or p4?

[BrennanConroy]

@bradygaster It's not really in my control. We are blocked on corefx getting the serializer and deserializer in.

[bradygaster]

@BrennanConroy ah ok - thanks for the info & context. will discuss during triage.

[natemcmaster]

I don't want to be hasty and make things too hard for customers. I'm trying to weigh this against front-loading breaking changes, such as removing Newtonsoft.Json from the framework.

If serializier/deserializer API's aren't coming in time for preview 3, there is an alternative. We could exclude Newtonsoft.Json from the targeting pack, but keep it in the runtime while SignalR waits for the new API. I think this lessens the impact while still moving us in the right direction. SignalR would still have the Json protocol implmentation in-box, but if customers wanted to use Newtonsoft.Json in other parts of their code, they would need to add a PackageReference to it.

The only problem with removing the ref and keeping the implementation is that SignalR exposes Json.Net types in two places:

Kind | Name | ReturnType | Assembly
---|---|---|---
Property | Microsoft.AspNetCore.SignalR.NewtonsoftJsonHubProtocolOptions.PayloadSerializerSettings | Newtonsoft.Json.JsonSerializerSettings | Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson
Property | Microsoft.AspNetCore.SignalR.Protocol.NewtonsoftJsonHubProtocol.PayloadSerializer | Newtonsoft.Json.JsonSerializer | Microsoft.AspNetCore.SignalR.Protocols.NewtonsoftJson

[muratg]

My opinion on adding a SignalR template: We should carefully analyze the cost/benefit ratio before deciding to have one. It's not clear to me what it even look like. Perhaps if scaffolding existed to add SignalR would be a good middle ground, but even that would have costs.

[muratg]

@natemcmaster Wouldn't that be more "hacky" in the build system? We should probably completely yank it out and tell people to add the referene manually. I believe we'll have some form of serialization/deserialization in P4. The problem of keeping it in in some form is that we know the full effects of removing it until P4.

[natemcmaster]

Hiding a reference assembly isn't actually too hacky. We might have to do this for other assemblies anyways, per https://github.com/dotnet/corefx/issues/34906#issuecomment-459540531

[bradygaster]

Perhaps if scaffolding existed to add SignalR would be a good middle ground, but even that would have costs.

I'd prefer just litter all the templates with "opt-in" functionality for SignalR, which'd drop in what they need in Startup.cs and a sample Hub class. Yes, there's a cost, but I think we should start thinking about when to pay that cost. I'll set up another issue for us to use to discuss this idea, so we can re-focus this thread on the issue at hand.

[davidfowl]

A SignalR template would be fine tbh, we resisted it for long but it would be a useful starting point as there's lots of moving pieces

[BrennanConroy]

Added Microsoft.Extensions.Logging.EventSource to the list at the top of this issue

cc @pakrym

[Tratcher]

Microsoft.AspNetCore.Authentication.* are done.

[natemcmaster]

This did not make it into preview3 and is being pushed out to preview4.

[HaoK]

Config PR merged https://github.com/aspnet/Extensions/pull/1028

[pakrym]

Logging done https://github.com/aspnet/Extensions/pull/1227.

[BrennanConroy]

FYI need to update ResolveReferences.targets file after this is done
https://github.com/aspnet/AspNetCore/blob/17344cd37f0b43b28c93c58cba667e55bd680624/eng/targets/ResolveReferences.targets#L88

[mkArtak]

@pranavkm can you please confirm whether this is done already?

we need to update Microsoft.AspNetCore.Mvc.ViewFeatures to make sure it doesn't pull in Newtonsoft.Json.