Make System.Text.Json the default for SignalR and remove Newtonsoft from shared framework #9476
Conversation
BrennanConroy
requested review from
analogrelay,
halter73,
mikaelm12 and
a team
as code owners
| @@ -457,12 +457,11 @@ describe("hubConnection", () => { | |||
|
|
|||
| const complexObject = { | |||
| ByteArray: protocol.name === "json" | |||
| ? "aGVsbG8=" | |||
| ? new Array(0x68, 0x65, 0x6c, 0x6c, 0x6f) | |||
There was a problem hiding this comment.
Filed https://github.com/dotnet/corefx/issues/36974 for this
| @@ -68,7 +68,6 @@ | |||
|
|
|||
| If these are needed as direct dependencies, it is okay to change them to ExternalAspNetCoreAppReference and move up into sections above. | |||
| --> | |||
| <_TransitiveExternalAspNetCoreAppReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonPackageVersion)" /> | |||
There was a problem hiding this comment.
@pranavkm @rynowak Is MVC good with this change? We are making the switch to System.Text.Json in SignalR by default now.
@ryanbrandenburg Do our templates reference Newtonsoft anywhere or did we already remove all that?
There was a problem hiding this comment.
What does this do?
Do our templates reference Newtonsoft anywhere or did we already remove all that?
MVC's templates still do. We were waiting for https://github.com/dotnet/corefx/issues/36024 to land since it affects quite a few of our tests
There was a problem hiding this comment.
This removes Newtonsoft.Json from the shared framework. We can't do that if MVC isn't ready to do it though.
My concern is that this is going to land super hot for preview 5 and I want everybody on the same page. Maybe we should leave Newtonsoft in the shared framework, even though we're changing the default.
There was a problem hiding this comment.
Ok, if we can land this soon that would be best. We need to try our templates after this change and make sure we haven't broken the immediate experience.
| @@ -25,7 +25,7 @@ public HubConnectionBuilder() | |||
| Services = new ServiceCollection(); | |||
| Services.AddSingleton<HubConnection>(); | |||
| Services.AddLogging(); | |||
| this.AddNewtonsoftJsonProtocol(); | |||
| this.AddJsonProtocol(); | |||
| @@ -457,12 +457,11 @@ describe("hubConnection", () => { | |||
|
|
|||
| const complexObject = { | |||
| ByteArray: protocol.name === "json" | |||
| ? "aGVsbG8=" | |||
| ? new Array(0x68, 0x65, 0x6c, 0x6c, 0x6f) | |||
There was a problem hiding this comment.
Maybe add a comment linking to the issue you filed for this.
| options.AllowTrailingCommas = false; | ||
| options.IgnoreNullValues = false; | ||
| options.IgnoreReadOnlyProperties = false; | ||
| // TODO: camelCase |
There was a problem hiding this comment.
Tracking this somewhere?
There was a problem hiding this comment.
Yes, in my head. I'll be doing this today so no need to track it.
|
Filed #9519 for options review |
Fixes #9439
Fixes #4260
Review: How do we want to expose json options? I'm currently wrapping the serializer options and passing the values through.