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

Support null @rendermode #9343

Open
halter73 opened this issue Oct 2, 2023 · 8 comments · Fixed by #9400
Open

Support null @rendermode #9343

halter73 opened this issue Oct 2, 2023 · 8 comments · Fixed by #9400
Assignees
Labels
area-compiler Umbrella for all compiler issues

Comments

@halter73
Copy link
Member

halter73 commented Oct 2, 2023

          > It looks we might need to change the runtime to provide a way to set the render mode to static server rendering.

Yes, I think you're right, and this is going to affect dotnet/aspnetcore#50920 (Auth with global interactivity) because it's exactly how we're most likely going to do it. cc @halter73

The reason it worked in my PR is probably that it didn't yet have the Razor compiler changes for @rendermode and hence was behaving as a regular attribute that gets elided if you pass null. We can change AddComponentRenderMode in the runtime to treat null as a no-op.

Originally posted by @SteveSandersonMS in dotnet/aspnetcore#51031 (comment)

@ghost ghost added the untriaged label Oct 2, 2023
@halter73 halter73 changed the title > It looks we might need to change the runtime to provide a way to set the render mode to static server rendering. Support null @rendermode Oct 2, 2023
@halter73
Copy link
Member Author

halter73 commented Oct 2, 2023

@SteveSandersonMS @danroth27 @chsienki @jaredpar

I think we're going to need to update the razor compiler to allow a null @rendermode for .NET 8 GA if we're sometimes going to be using null in our .NET 8 project templates. Otherwise, users will see errors like the following warning when building a new project template unless we sprinkle in a deceitful !:

Components\App.razor(13,106): warning CS8600: Converting null literal or possible null value to non-nullable type.
Components\App.razor(17,105): warning CS8600: Converting null literal or possible null value to non-nullable type.

@chsienki
Copy link
Contributor

chsienki commented Oct 2, 2023

Is that not just the underlying API needs the annotation applied to it to allow a nullable type?

@SteveSandersonMS
Copy link
Member

Is that not just the underlying API needs the annotation applied to it to allow a nullable type?

Yeah I would have thought so. Not sure why a compiler change would be required, but if @halter73 knows it is, please clarify!

@halter73
Copy link
Member Author

halter73 commented Oct 3, 2023

I've updated RenderTreeBuilder.AddComponentRenderMode(IComponentRenderMode renderMode) to RenderTreeBuilder.AddComponentRenderMode(IComponentRenderMode? renderMode) in the template update branch, but this does not silence the warnings from App.razor's <Routes @rendermode="null" /> (effectively).

I'm not 100% sure why this still leads to CS8600 warning, but could it because the code generator is still attempting to treat the IComponentRenderMode as non-nullable? For example:

Content = $"public override global::{ComponentsApi.IComponentRenderMode.FullTypeName} Mode => ModeImpl;"

Or does @rendermode's parameter type flow directly from the public runtime AddComponentRenderMode API? That seems unlikely, and is not what I'm seeing locally.

@chsienki
Copy link
Contributor

chsienki commented Oct 3, 2023

@halter73 Ah you're right, its because we do the double assign to avoid side effects that this crops up. We can either make this nullable too (seems correct, if the underlying API is nullable) or not emit in the case of null.

@jaredpar jaredpar added this to the 17.8 Planning milestone Oct 3, 2023
@ghost ghost removed the untriaged label Oct 3, 2023
@jaredpar
Copy link
Member

jaredpar commented Oct 3, 2023

Do we need to be concerned about ComponentsApi.IComponentRenderMode.FullTypeName representing say a struct type? Basically do we need have a bit of smarts or can we just append ? to the type name.

@halter73
Copy link
Member Author

halter73 commented Oct 4, 2023

I'm pretty sure you can rely on it being a class, but I'll defer to @SteveSandersonMS.

@SteveSandersonMS
Copy link
Member

Just appending ? to the type name seems fine. IComponentRenderMode itself will always be an interface, and the only implementations that the runtime cares about are classes.

@phil-allen-msft phil-allen-msft added the area-compiler Umbrella for all compiler issues label Oct 5, 2023
mkArtakMSFT pushed a commit to dotnet/aspnetcore that referenced this issue Oct 11, 2023
## 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
@chsienki chsienki modified the milestones: 17.8 Planning, 17.8 P3 Oct 11, 2023
@chsienki chsienki self-assigned this Feb 10, 2024
@chsienki chsienki modified the milestones: 17.8, 17.10 Planning Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants