Skip to content

Conversation

agocke
Copy link
Member

@agocke agocke commented Aug 7, 2025

Reverts #63099

Working around trimming issues with dynamicdependency is not reliable. Our documented way to fix trimming issues is by turning on warnings and solving the trim warnings.

@Copilot Copilot AI review requested due to automatic review settings August 7, 2025 15:10
@agocke agocke requested a review from a team as a code owner August 7, 2025 15:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reverts a previous change that added a DynamicDependency attribute to preserve KeyValuePair for WebAssembly trimming scenarios. The revert removes trimming preservation logic that was previously added to ensure KeyValuePair types remain available during assembly trimming in WebAssembly applications.

  • Removes DynamicDependency attribute for KeyValuePair JSON serialization preservation
Comments suppressed due to low confidence (1)

src/Components/WebAssembly/WebAssembly/src/Services/DefaultWebAssemblyJSRuntime.cs:33

  • Removing this DynamicDependency attribute may cause KeyValuePair types to be trimmed away in WebAssembly applications, potentially breaking JSON serialization functionality. Consider documenting why this revert is safe or if alternative trimming preservation measures are in place.
    [DynamicDependency(nameof(UpdateRootComponentsCore))]

@github-actions github-actions bot added the area-blazor Includes: Blazor, Razor Components label Aug 7, 2025
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

I understand the concerns about trimming reliability, but I believe we need to consider the broader impact on our customers here.

Blazor has supported partial trimming since its initial release, with JS interop using System.Text.Json internally from the beginning - well before the S.T.J source generator existed. Many of our customers have built applications around this established pattern, using runtime reflection for serialization in their JS interop scenarios.

Moving to source generation would be a significant breaking change that would require substantial refactoring of existing, working applications. There are immediate scenarios where source generation would make the developer experience worse:

  1. Annotation burden: Customers would need to annotate every type used in JS interop with [JsonSerializable], including nested types and dependencies. This is particularly painful for types from third-party libraries where they don't control the source.

  2. Razor component limitations: Types defined within .razor files (a common pattern for component-specific DTOs and view models) cannot participate in source generation since they're generated at build time. This would force developers to move these types to separate .cs files, breaking the component encapsulation that makes Blazor productive.

DynamicDependency is part of our documented trimming guidance, and using documented APIs to preserve functionality seems like a reasonable approach to maintain compatibility.

It's worth noting that Blazor's current trimming strategy (trimming framework assemblies but not user code) has been working well for our scenarios, and we haven't seen compelling evidence that full trimming would provide significant benefits for typical Blazor applications.

I strongly believe we should prioritize our customers' ability to upgrade without breaking their existing code. The original fix in #63099 solves a real problem that customers are facing today. Removing this fix would leave customers stuck between staying on an older version or undertaking significant refactoring work that doesn't add business value.

This fix maintains compatibility while keeping Blazor functional for our existing user base. That's the right tradeoff for our framework.

@javiercn javiercn closed this Aug 7, 2025
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-rc1 milestone Aug 7, 2025
@agocke
Copy link
Member Author

agocke commented Aug 7, 2025

As mentioned, the behavior of DynamicDependency is undocumented and unreliable. If the behavior changes in a future release, this fix may no longer work.

@agocke agocke deleted the revert-63099-copilot/fix-63098 branch August 7, 2025 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants