Make RenderTreeBuilder.AddComponentRenderMode's renderMode parameter nullable #51170

halter73 · 2023-10-05T18:36:48Z

Background and Motivation

To support disabling interactivity on a per-page basis with the help of @rendermode="null" for use in our project templates when generating a Blazor app with individual auth and global interactivity. See #51134.

Proposed API

namespace Microsoft.AspNetCore.Components.Rendering;

public sealed class RenderTreeBuilder : IDisposable
{
-    public void AddComponentRenderMode(IComponentRenderMode renderMode);
+    public void AddComponentRenderMode(IComponentRenderMode? renderMode);
}

Usage Examples

App.razor

<!DOCTYPE html>
<html lang="en">

<head>
    <!-- ... -->
    <HeadOutlet @rendermode="@RenderModeForPage" />
</head>

<body>
    <Routes @rendermode="@RenderModeForPage" />
    <script src="_framework/blazor.web.js"></script>
</body>

</html>

@code {
    [CascadingParameter] HttpContext HttpContext { get; set; } = default!;

    IComponentRenderMode? RenderModeForPage => HttpContext.Request.Path.StartsWithSegments("/Account")
        ? null
        : RenderMode.InteractiveServer;
}

Alternative Designs

We could add a new RenderMode.StaticServer to mean the same thing that null does with this proposal.

We could also look at other public API that accept a non-nullable render mode like RenderModeAttribute:

namespace Microsoft.AspNetCore.Components;

[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
public abstract class RenderModeAttribute : Attribute
{
-    public abstract IComponentRenderMode Mode { get; }
+    public abstract IComponentRenderMode? Mode { get; }
}

This could be useful for forcing a component to only render statically.

Risks

It's risky making API changes after RC2 because that means we have no opportunity to change the API again based on customer feedback. I think this risk is minimized because it's only a change to the nullability annotation and it makes it more relaxed.

Perhaps the bigger risk is that it locks us into null representing a non-interactive renter mode. This is already the case internally, but that could be confusing if we wanted to add something like RenderMode.StaticServer in the future to represent the same thing. At least that could still work though.

The text was updated successfully, but these errors were encountered:

ghost · 2023-10-05T18:40:21Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

augustevn · 2023-10-11T12:48:26Z

I'm suddenly getting the error while building RC1 Blazor. Without changing anything since the day before.
So, I upgraded to RC2 but Azure App Service is missing that SDK.
I can't go back and can't go forward.

There is no argument give that correspons to the required parameter 'renderMode' of 'RenderTreeBuilder.AddComponentRenderMode(int, IComponentRenderMode)'

halter73 · 2023-10-11T20:22:35Z

API Review Notes:

Perhaps the bigger risk is that it locks us into null representing a non-interactive renter mode.

@SteveSandersonMS pointed out that "I’d define it as meaning “no op”, i.e., telling the framework to just continue with whatever rendermode it was otherwise going to use. A no-op is literally what the implementation will do internally, right? That doesn’t seem so opinionated and doesn’t preclude an explicit “Static” rendermode switch in the future."

but I wonder if we shouldn't also change RenderModeAttribute.Mode to also be nullable

@SteveSandersonMS also responded to this with "While it would be marginally more consistent to do that, it’s semantically more complex because we’d also have to change the runtime code that searches for RenderModeAttribute on the type hierarchy and make it ignore ones with Mode=null (otherwise it would not be a no-op). Since we have no use case for this, and to avoid runtime churn at this point I’d be happy enough to leave RenderModeAttribute.Mode alone. Yes it would technically be a breaking change if we later made this nullable, but it seems unlikely to happen."

Without any further feedback, I updated #51134 to have AddComponentRenderMode literally no-op rather than add a null render mode to the render tree. I agree that making RenderModeAttribute.Mode nullable seems unessary. I just wanted to make sure we considered it.

API Approved!

namespace Microsoft.AspNetCore.Components.Rendering;

public sealed class RenderTreeBuilder : IDisposable
{
-    public void AddComponentRenderMode(IComponentRenderMode renderMode);
+    public void AddComponentRenderMode(IComponentRenderMode? renderMode);
}

@SteveSandersonMS

## Description This PR addresses a large amount of feedback from #50722 which was merged before they could all be addressed to unblock Accessibility Testing effort. The primary impacts are: #### Runtime changes - Public API change to make `AddComponentRenderMode`'s `renderMode` param nullable to support disabling interactivity on a per-page basis with the help of `@rendermode="null"` (effectively). - **IMPORTANT:** This will need follow up in the Razor Compiler. See dotnet/razor#9343 - API Proposal issue: #51170 - This is a e necessary to support the changes to add global interactivity to Identity components @SteveSandersonMS made in #50920 and have now been included in this PR. - [Add antiforgery token to forms rendered interactively on the server](425bd12) - This bug fix is necessary to make the logout button work without throwing antiforgery errors when it is rendered interactively on the server. #### Template changes - Fix compilation error due to missing `using` in `Program.cs` when the individual auth option is selected with no interactivity. - Add support for global (`--all-interactive`) instead of just per-page interactivity to the new Identity components. - Fix "Apply Migrations" link on the `DatabaseErrorPage` by calling `UseMigrationsEndPoint()` when necessary. - Add support for non-root base paths to the new Identity components. - Improve folder layout by putting most of the additional auth and Identity related files in the same /Account folder. - Use the new `IEmailSender<ApplicationUser>` API instead of `IEmailSender` for easier customization of emails. - Remove usage of `IHttpContextAccessor` from the template because that is generally regarded as bad practice due to the unnecessary reliance on `AsyncLocal`. - Remove underscore (`_`) from private field names. - Reduce usage of `null!` and `default!`. - Use normal `<button>` for logout link in nav bar rather than `<a onclick="document.getElementById('logout-form').submit();">`, and remove separate `LogoutForm.razor`. ## Customer Impact This fixes several bugs in the Blazor project template when choosing the individual auth option and makes several runtime fixes that will be beneficial to any global interactive Blazor application that needs to include some components that must always be statically rendered. ## Regression? - [ ] Yes - [x] No ## Risk - [ ] High - [ ] Medium - [x] Low Obviously, we would rather not make such a large change after RC2. Particularly when it's a change that touches public API. Fortunately, the runtime changes are very small, and only to parts of the runtime that were last updated in RC2 (see #50181 and #50946). The vast majority of the changes in the PR only affect the Blazor project template when the non-default individual auth option is selected. This was merged very late in RC2 (#50722) with the expectation that we would make major changes prior to GA. ## Verification - [x] Manual (required) - [x] Automated ## Packaging changes reviewed? - [ ] Yes - [ ] No - [x] N/A

halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 5, 2023

dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Oct 5, 2023

halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 5, 2023

halter73 added this to the 8.0.0 milestone Oct 5, 2023

halter73 mentioned this issue Oct 5, 2023

Update Identity Components in Blazor project template #51134

Merged

10 tasks

augustevn mentioned this issue Oct 11, 2023

.NET 8 RC1 Blazor build suddenly errors. Required parameter 'renderMode' of ... #51293

Closed

1 task

halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 11, 2023

halter73 closed this as completed Oct 11, 2023

mkArtakMSFT assigned halter73 Oct 12, 2023

ghost locked as resolved and limited conversation to collaborators Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make RenderTreeBuilder.AddComponentRenderMode's renderMode parameter nullable #51170

Make RenderTreeBuilder.AddComponentRenderMode's renderMode parameter nullable #51170

halter73 commented Oct 5, 2023 •

edited

Loading

ghost commented Oct 5, 2023

augustevn commented Oct 11, 2023 •

edited

Loading

halter73 commented Oct 11, 2023

Make RenderTreeBuilder.AddComponentRenderMode's renderMode parameter nullable #51170

Make RenderTreeBuilder.AddComponentRenderMode's renderMode parameter nullable #51170

Comments

halter73 commented Oct 5, 2023 • edited Loading

Background and Motivation

Proposed API

Usage Examples

App.razor

Alternative Designs

Risks

ghost commented Oct 5, 2023

augustevn commented Oct 11, 2023 • edited Loading

halter73 commented Oct 11, 2023

halter73 commented Oct 5, 2023 •

edited

Loading

augustevn commented Oct 11, 2023 •

edited

Loading