Skip to content

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented May 12, 2020

Fixes #17160

@JamesNK JamesNK requested review from Tratcher and jkotalik as code owners May 12, 2020 06:49
@ghost ghost added the area-servers label May 12, 2020
@JamesNK JamesNK requested review from BrennanConroy and rynowak May 12, 2020 06:49
@JamesNK
Copy link
Member Author

JamesNK commented May 15, 2020

I've added support for MVC's JsonOptions to use Http.Json's JsonOptions as its base.

Does it make sense for SignalR to do the same thing? SignalR's JSON protocol is independent of ASP.NET Core. It can run on the server or the client.

@davidfowl
Copy link
Member

Does it make sense for SignalR to do the same thing? SignalR's JSON protocol is independent of ASP.NET Core. It can run on the server or the client.

I'm on the fence, I think customers do want a single way to configure the server defaults. @BrennanConroy and @halter73 any suggestions?

@JamesNK
Copy link
Member Author

JamesNK commented May 19, 2020

If SignalR did look to JsonOptions as a source of truth, does a client created on a server pick up what the server is configured when serializing its messages as JSON to send to another server?

e.g. you're calling SignalR from a server and the server has been configured so that Enum instances are serialized as strings. Does the SignalR client automatically start serializing messages with Enum instances as strings?

I think this would be unexpected behavior.

@davidfowl
Copy link
Member

If SignalR did look to JsonOptions as a source of truth, does a client created on a server pick up what the server is configured when serializing its messages as JSON to send to another server?

Maybe just the server? But this brings up more questions about what the HttpClient factory should do or rather, how to make it read these same options.

I think we should leave this alone and separate from SignalR and MVC. If MVC happen to use this API then it will get the appropriate options but we shouldn't import these options everywhere.

That's my current take until we get more feedback.

@halter73
Copy link
Member

Even on the server, I think it should be possible for MVC and SignalR to use different JSON options.

@JamesNK JamesNK force-pushed the jamesnk/json-extensions branch from 81efed8 to c9db22c Compare May 28, 2020 05:52
@JamesNK JamesNK marked this pull request as ready for review May 28, 2020 09:52
@JamesNK
Copy link
Member Author

JamesNK commented May 28, 2020

🆙 📅

@pranavkm
Copy link
Contributor

You might have to use https://github.com/dotnet/aspnetcore/pull/22275/files#diff-157077aa157aa701d4b4a1419f32a298R1 trick to get the ref assembly working. #22323 should fix it for all ref assemblies.

@JamesNK JamesNK changed the base branch from master to release/5.0-preview6 June 2, 2020 00:04
@JamesNK JamesNK force-pushed the jamesnk/json-extensions branch from 0a6fe95 to 7da22a5 Compare June 2, 2020 00:07
@JamesNK JamesNK added this to the 5.0.0-preview6 milestone Jun 2, 2020
@ghost
Copy link

ghost commented Jun 2, 2020

Hello @JamesNK!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link

ghost commented Jun 2, 2020

Apologies, while this PR appears ready to be merged, it looks like release/5.0-preview6 is a protected branch and I have not been granted permission to perform the merge.

@JamesNK JamesNK removed the auto-merge label Jun 2, 2020
@Pilchie Pilchie merged commit 4c7e313 into release/5.0-preview6 Jun 2, 2020
@Pilchie Pilchie deleted the jamesnk/json-extensions branch June 2, 2020 04:00
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HttpContext and JSON

10 participants