Skip to content

Conversation

BrennanConroy
Copy link
Member

Fixes #9519

@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label May 7, 2019
@BrennanConroy BrennanConroy added this to the 3.0.0-preview6 milestone May 7, 2019
options.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
options.PropertyNameCaseInsensitive = true;
options.MaxDepth = 64;
options.DictionaryKeyPolicy = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default, but if you want to be explicit then that's ok too

options.IgnoreNullValues = false;
options.IgnoreReadOnlyProperties = false;
options.PropertyNamingPolicy = JsonNamingPolicy.CamelCase;
options.PropertyNameCaseInsensitive = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe MVC is leaving this as false. Should there be consistency between ASP.NET Core frameworks?

// @rynowak

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we were leaving this as false. I'm interested to know why folks think case-insensitive is a better option 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's because Json.NET is true by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ryan is making a stand that Json.NET has the wrong default. I don't feel strongly either way other than SignalR and MVC should be consistent

#TeamConsistency

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel incredibly strongly about SignalR and MVC being consistent. I do feel strongly that case-insensitive is the wrong choice for building a REST api.

I think it's a wierd idea to we would do case-insenstive because legacy.

Copy link
Contributor

@analogrelay analogrelay May 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as JsonNamingPolicy.CamelCase affects reading (IIRC the equivalent didn't in Json.NET?) I'm OK with leaving this false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it affects reading. {"someName":"value"} will serialize into class C { public string SomeName { get; set; } with CamelCase set, and doesn't if it isn't.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The policy effects serializing and deserializing.

@BrennanConroy
Copy link
Member Author

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com.

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/1929
https://github.com/aspnet/AspNetCore-Internal/issues/2451
https://github.com/aspnet/AspNetCore-Internal/issues/2250

@BrennanConroy
Copy link
Member Author

I am unable to merge this change because the blazor tests are too flaky...

@SteveSandersonMS I think your recent change to make tests more reliable might have made them more unreliable? e0007f4

@SteveSandersonMS
Copy link
Member

@BrennanConroy Sorry for the inconvenience.

Something has changed to make the components E2E tests start being flaky since yesterday, but I don't know what. It started before I made the change you mention (my change was an attempt to address it, but didn't).

I've been investigating extensively and have got as far as this, which might fix it.

@BrennanConroy BrennanConroy merged commit 9c32660 into master May 9, 2019
@ghost ghost deleted the brecon/jsonApi branch May 9, 2019 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-signalr Includes: SignalR clients and servers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apply API Review feedback to JsonHubProtocolOptions

8 participants