Make System.Text.Json the default for SignalR and remove Newtonsoft from shared framework#9476
Conversation
natemcmaster
left a comment
There was a problem hiding this comment.
Approved w.r.t. shared framework changes.
| 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
|
|
||
| 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.
| Services.AddSingleton<HubConnection>(); | ||
| Services.AddLogging(); | ||
| this.AddNewtonsoftJsonProtocol(); | ||
| this.AddJsonProtocol(); |
| 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.
mikaelm12
left a comment
There was a problem hiding this comment.
File an issue for reviewing the options.
|
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.