Prevent LayoutComponentBase properties from being trimmed. #30606

pranavkm · 2021-03-03T19:18:39Z

Note that this SDK is busted for Blazor apps. We'll need to pick up another SDK, but the source changes should be the same.

HaoK · 2021-03-03T19:30:06Z

src/Components/Components/src/LayoutComponentBase.cs

+            // it appears unused. Statically reference it to ensure the setter is preserved.
+            if (parameters.TryGetValue(BodyPropertyName, out RenderFragment? body))
+            {
+                Body = body;


This is a new side effect where Body is updated automatically when this is called, which I'm guessing is good/intentional?

base.SetParametersAsync effectively does this, but using reflection. In fact, we're actually assigning to this property twice but doing it this way means we're not introducing any new behavior to LayoutComponentBase, and the assignment is statically visible to the trimmer which means it'll keep the setter if this method appears in code.

Ah ok that makes sense then, do we still need to call base then? Does the base do anything more than set the body?

SetParametersAsync handles the entire lifetime of the component (including calling OnInitialized etc), so you do need to call it. Ideally we would want to avoid the reflection based assignment by doing something like this - https://docs.microsoft.com/en-us/aspnet/core/blazor/webassembly-performance-best-practices?view=aspnetcore-5.0#implement-setparametersasync-manually, but because this component is meant to be derived, we don't know if the derived type has additional parameters that need to be assigned.

Cool thanks for the explanation!

@pranavkm To make the flow conceptually simpler, and avoid the double-write oddness, the pattern I've done in my own components when overriding SetParametersAsync is this:

public override Task SetParametersAsync(ParameterView parameters) { // ... write all my own parameter properties ... return base.SetParametersAsync(ParameterView.Empty); }

I think this is the code pattern we'd generate if we did the source-generators-for-parameter-assignment work. Might be worth going with it here too.

Oh wait the point you make about subclassing complicates this. We'd probably be OK in the case of a layout, since there's no normal way to pass parameters to layouts. But in principle people might do weird stuff and it would be better not to block them. I guess you should not pass ParameterView.Empty to base.

As for the more general case with other components and source generators, I'm not sure how we'll handle this. We could argue that subclasses should also override SetParametersAsync to assign their own derived properties, but then they can't simply pass ParameterView.Empty to the base.

We might need to make the Razor compiler know whether you're directly subclassing ComponentBase or something else. If you are, it's safe to pass ParameterView.Empty to the base, and you get the perf benefit. But if your component inherits something else, we don't know whether or not to do that, and you don't get the perf benefit.

Clearly that's not a great design. We might need something more complicated.

For the source generator, the way I had thought of it was we would code-generate some structure that would replace or feed in to WritersForType. We would simply replace the reflection discovery / invoke with compiled structures so we avoid some of the costs there.

src/Components/WebAssembly/testassets/WasmLinkerTest/WasmLinkerTest.csproj

src/Components/Components/src/LayoutComponentBase.cs

pranavkm · 2021-03-04T16:45:32Z

@captainsafia any idea why there isn't an installer with your SDK changes as yet? The most recent installer is about a day old.

captainsafia · 2021-03-04T20:00:09Z

@captainsafia any idea why there isn't an installer with your SDK changes as yet? The most recent installer is about a day old.

They just fixed the bug in the installer repo that was causing this so should hopefully have SDK -> installer updates flowing soon.

pranavkm · 2021-03-06T18:45:09Z

@captainsafia - https://dev.azure.com/dnceng/public/_build/results?buildId=1025923&view=ms.vss-test-web.build-test-results-tab&runId=31881838&resultId=125311&paneView=debug

C:\h\w\B1000931\w\B2770998\e\sdk338\x64\sdk\6.0.100-preview.3.21155.4\MSBuild.dll -distributedlogger:Microsoft.DotNet.Tools.MSBuild.MSBuildLogger,C:\h\w\B1000931\w\B2770998\e\sdk338\x64\sdk\6.0.100-preview.3.21155.4\dotnet.dll*Microsoft.DotNet.Tools.MSBuild.MSBuildForwardingLogger,C:\h\w\B1000931\w\B2770998\e\sdk338\x64\sdk\6.0.100-preview.3.21155.4\dotnet.dll -maxcpucount -property:Configuration=Release -target:Publish -verbosity:m /bl .\AspNet.n1bwsxr4lfc.Server.csproj
You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
AspNet.n1bwsxr4lfc.Shared -> C:\h\w\B1000931\w\B2770998\e\Templates\BaseFolder\AspNet.n1bwsxr4lfc\Shared\bin\Release\net6.0\AspNet.n1bwsxr4lfc.Shared.dll
CSC : warning CS8785: Generator 'RazorSourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'AggregateException' with message 'One or more errors occurred. (Object reference not set to an instance of an object.)' [C:\h\w\B1000931\w\B2770998\e\Templates\BaseFolder\AspNet.n1bwsxr4lfc\Client\AspNet.n1bwsxr4lfc.Client.csproj]
C:\h\w\B1000931\w\B2770998\e\Templates\BaseFolder\AspNet.n1bwsxr4lfc\Client\Program.cs(18,40): error CS0246: The type or namespace name 'App' could not be found (are you missing a using directive or an assembly reference?) [C:\h\w\B1000931\w\B2770998\e\Templates\BaseFolder\AspNet.n1bwsxr4lfc\Client\AspNet.n1bwsxr4lfc.Client.csproj]

Expected: True

pranavkm · 2021-03-06T19:10:28Z

src/Components/Shared/src/WebEventData.cs

@@ -57,6 +57,18 @@ private WebEventData(int browserRendererId, ulong eventHandlerId, EventFieldInfo

        public EventArgs EventArgs { get; }

+        [DynamicDependency(JsonSerialized, typeof(ChangeEventArgs))]


I imagine the need to do this would go away once we have the JSON code generator. In the meanwhile, I ran the BasicTestApp locally and all of the scenarios worked. Working on getting tests sorted next.

I'm OK with this being here, especially if it's temporary, but it is strange.

It's not as if the developer manually creates some chain of calls that lead here. It's not really a dynamic dependency. In reality the framework hard-codes the idea that the event dispatch code paths are reachable, so it's very arbitrary that this particular method is where we put the rules about retaining ClipboardEventArgs et al. It would be potentially difficult to find these attributes later if we wanted, since there's no clear logical deduction a developer could make to realise they would be here.

If there was an alternative to annotate the eventargs subclasses directly to say "never trim this" I think that would be a lot clearer. Don't know if there is such a mechanism like the following:

[NeverTrim] public class ClipboardEventArgs : EventArgs { ... }

If there was an alternative to annotate the eventargs subclasses directly to say "never trim this" I think that would be a lot clearer.

AFAIK, there isn't a non-XML way to do this. That said, I was able to modify the code to avoid any trimmer specific by invoking the JSON serialization inline.

The usage pattern for LayoutComponentBase does not really play well with static analysis and results in the property setter from being removed.

pranavkm · 2021-03-08T18:00:16Z

@captainsafia I'm backing out the SDK change for now because it's failing with the error I linked to earlier. Could you update it once your diagnostics changes are in?

pranavkm requested a review from a team March 3, 2021 19:18

HaoK reviewed Mar 3, 2021

View reviewed changes

captainsafia reviewed Mar 3, 2021

View reviewed changes

src/Components/WebAssembly/testassets/WasmLinkerTest/WasmLinkerTest.csproj Outdated Show resolved Hide resolved

Pilchie added the area-blazor Includes: Blazor, Razor Components label Mar 3, 2021

HaoK approved these changes Mar 3, 2021

View reviewed changes

SteveSandersonMS reviewed Mar 4, 2021

View reviewed changes

src/Components/Components/src/LayoutComponentBase.cs Outdated Show resolved Hide resolved

pranavkm force-pushed the prkrishn/trimming branch from df3d769 to d437e80 Compare March 6, 2021 15:32

pranavkm marked this pull request as ready for review March 6, 2021 19:09

pranavkm requested a review from a team as a code owner March 6, 2021 19:09

pranavkm commented Mar 6, 2021

View reviewed changes

SteveSandersonMS approved these changes Mar 8, 2021

View reviewed changes

pranavkm force-pushed the prkrishn/trimming branch from d437e80 to 07091da Compare March 8, 2021 17:40

Prevent LayoutComponentBase properties from being trimmed.

cd11dcb

The usage pattern for LayoutComponentBase does not really play well with static analysis and results in the property setter from being removed.

pranavkm force-pushed the prkrishn/trimming branch from 07091da to cd11dcb Compare March 8, 2021 17:41

pranavkm merged commit bf51aa5 into main Mar 8, 2021

pranavkm deleted the prkrishn/trimming branch March 8, 2021 19:41

pranavkm added this to the 6.0-preview3 milestone Mar 8, 2021

pranavkm mentioned this pull request Mar 8, 2021

[Blazor WebAssembly] LayoutComponentBase.Body setter is being trimmed away #30605

Closed

		@@ -57,6 +57,18 @@ private WebEventData(int browserRendererId, ulong eventHandlerId, EventFieldInfo

		public EventArgs EventArgs { get; }

		[DynamicDependency(JsonSerialized, typeof(ChangeEventArgs))]

Prevent LayoutComponentBase properties from being trimmed. #30606

Prevent LayoutComponentBase properties from being trimmed. #30606

Uh oh!

Conversation

pranavkm commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pranavkm commented Mar 4, 2021

Uh oh!

captainsafia commented Mar 4, 2021

Uh oh!

pranavkm commented Mar 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Mar 8, 2021

Uh oh!

Uh oh!

pranavkm Mar 3, 2021 •

edited

Loading

SteveSandersonMS Mar 4, 2021 •

edited

Loading

SteveSandersonMS Mar 4, 2021 •

edited

Loading

SteveSandersonMS Mar 8, 2021 •

edited

Loading