Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default JsonSerializerIsReflectionEnabledByDefault to true on Blazor WASM. #31909

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

eerhardt
Copy link
Member

Since #31626, the Web SDK is defaulting JsonSerializerIsReflectionEnabledByDefault=false whenever PublishTrimmed or PublishAot is set to true. However, this also affects Blazor WASM since it is importing the Web SDK. For Blazor WASM, we need to keep the normal default of JsonSerializerIsReflectionEnabledByDefault=true, so it doesn't break existing Blazor WASM apps that are relying on JsonSerializer using Reflection.

See discussion in dotnet/runtime#84378.

NOTE: I don't know how to test this at this layer. If anyone has a good recommendation for how to test it, I am open to suggestions.

@eerhardt eerhardt requested a review from a team as a code owner April 20, 2023 15:12
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member labels Apr 20, 2023
@ghost
Copy link

ghost commented Apr 20, 2023

Thanks for your PR, @eerhardt.
To learn about the PR process and branching schedule of this repo, please take a look at the SDK PR Guide.

@eerhardt eerhardt requested a review from JamesNK April 20, 2023 15:14
@@ -17,6 +17,9 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Trimmer defaults that depend on user-definable settings.
This must be configured before it's initialized in the .NET SDK targets (which are imported by the Razor SDK). -->
<SuppressTrimAnalysisWarnings Condition="'$(SuppressTrimAnalysisWarnings)' == '' And '$(TrimmerDefaultAction)' != 'link'">true</SuppressTrimAnalysisWarnings>

<!-- Similarly these feature switches must be configured before they are initialized in imported SDKs -->
<JsonSerializerIsReflectionEnabledByDefault Condition="'$(JsonSerializerIsReflectionEnabledByDefault)' == ''">true</JsonSerializerIsReflectionEnabledByDefault>
Copy link
Member

Choose a reason for hiding this comment

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

Does this need any testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

See the note in the top comment.

NOTE: I don't know how to test this at this layer. If anyone has a good recommendation for how to test it, I am open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK blazor apps don't run here. So best way to test it is to locally run blazor app with the built SDK.

Copy link
Member

Choose a reason for hiding this comment

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

This can't be disabled for blazor apps, if the user disables this, we should emit an error instead in the context of blazor webassembly apps or simply ignore the user choice.

The framework is dependent on this feature for basic functionality (JS interop)

@lewing lewing requested a review from maraf April 20, 2023 16:05
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

It's ok to leave this change affecting only blazor, not vanilla wasm SDK, since that doesn't have an existing user base expecting it to work this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants