dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.18k stars 9.93k forks source link

Improve Razor Rendering with Null References #57386

Closed garrettlondon1 closed 3 weeks ago

garrettlondon1 commented 3 weeks ago

Summary

When working with Blazor, non-stream rendered components should fix the way they render, so that OnInitializes finishes before the razor view is handled

Motivation and goals

Beginner developers should not have to litter the code with ? checks when not stream rendering. OnInitialized should run before the view is rendered like below.

Below will throw a null reference, even though OnInitialized is above the razor view, if the view is not stream rendered, developer should not have to check the nullability of objects in the razor views.

I do not think this code should throw a NRE even if Animal is null. Yes, we could initialize the object, but I don't think that's an elegant solution and not everything can be initialized

Screenshot 2024-08-17 at 6 39 45 PM

In scope

Run OnInitialized before view renders in razor components that are not stream rendered. Don't force developers to null check every single condition that will eventually be solved under the OnInitializedAsync call.

This applies to everything except for stream rendered components. SSR components not stream rendered + minimal APIs

iphdav commented 3 weeks ago

@garrettlondon1 I would strongly disagree with this proposal. I think the current behavior is one of the biggest benefits of Blazor. It is extremely common for my code to asynchronously aggregate lots of information inside the OnInitializeAsync method. Blazor is fantastic in that you can call a StateHasChanged() and have it perform a re-render multiple times before getting to the end of OnInitializeAsync, after which a final render is automatically performed.

This allows pages that bring in 4-5 different artefacts asynchronously, refreshing what is displayed as soon as each bit of backend data is available.

This places Blazor way ahead of other frameworks like React. Even if you look at the (beta) React Server Components. It is somewhat janky in what they are trying to do, such as either returning a promise the then needs to be serialized as JSON to the browser; or using a Suspense component to later generate delayed HTML to be sent to the client. This is all so much less elegant than what Blazor does out of the box, because unlike Blazor, React Server Components can only render once. You cannot re-render during init multiple times (on the server) - please don't take this away from Blazor it is one of the highlights of the framework.

If @garrettlondon1 wants a work-around, he can use a field bool loaded = false; and as the last line in his OnInitializedAsync he can use loaded = true;. He can then use a few options such as: Use IF statement

@if (loaded) {
   <div>Markup here</div>
}

If you do not like the indentation you can do this at the top:

@if (!loaded) { return; }

Finally you can use a Display.razor component like this:

@if (Loaded) {
    @ChildContent
}
[CascadingParameter] bool Loaded {get; set;}
[CascadingParameter] RenderFragment ChildContent {get; set}

You can then use it in your pages like this:

<Display Loaded=loaded>
    <div>Rest of markup</div>
</Display>
@code {
   bool loaded;
   protected override async Task OnInitializedAsync()
   {
       \\code
       loaded = true; \\ set on last line
   }
}

You could even have a base class that required loaded to be set true like this:

@inherits DelayRenderBaseComponent

Then require you to set Loaded = true; (property of the base class) for rendering to start. EDIT: Or this new base component could, by default, do what you are asking (delay rendering until OnInitializeAsync is complete) without needing a base.Loaded=true; to be set.

I think something more constructive @garrettlondon1 would be to suggest a feature that could make one of the above scenarios more automatic.

iphdav commented 3 weeks ago

@garrettlondon1 I doubt the team will want to implement anything here, but maybe your best option would be to request something like the following.

Today when using OnInitializeAsync, without adding additional StateHasChanged() calls, a render will occur both: a) The first time you await an asynchronous result. b) After OnInitializeAsync completes.

Maybe you should request the default base class support a new directive like such:

@delayrender

And if that directive is at the top of the page, the first render above (when awaiting an asynchronous result) marked as "a" is skipped.

That is probably the closest you will get to something the team would seriously consider.

garrettlondon1 commented 3 weeks ago

This allows pages that bring in 4-5 different artefacts asynchronously, refreshing what is displayed as soon as each bit of backend data is available.

This is not true @iphdav

This only applies to stream rendering and OnInitialized

StateHasChanged() and Loading indicator has no relevance to this story.

StateHasChanged() and Loading indicator cannot be applied to a SSR Page without stream rendering

Without Stream Rendering, a Loading indicator will only prevent the Null References, it will not do anything else to make the page load better

Non-stream rendered components should only render once: OnInitialized First, then the view

With @attribute [StreamRendering]

Without that attribute, this is the point of the whole story

iphdav commented 3 weeks ago

I have have misunderstood you when you said "This applies to everything", but now I understand you are not talking about the interactive modes (you are not talking about InteractiveServer or InteractiveWebAssembly). You are only talking about SSR with non-streaming.

But I still disagree with what you are suggesting because of the developer confusion when changing render modes. You are suggesting something that would allow @animal.Name to work in a very specific scenario (SSR non-streaming, and non-interactive) where that same code would break in almost any other render mode. We should keep things simple and consistent for people learning the platform.

But also, if you don't allow the initial render, how do the sub-components kick off that I might want to setup at the start of OnInitializeAsync?

<div>@animal.Name</div>
@foreach (var item in items)
{
    <SomeOtherComponent Item="item" />
}

I want that to start immediately, even though you cannot see anything.

There may be a bunch of other async stuff that SomeOtherComponent needs to do and I want that starts as soon as possible thank you very much.

This is not going to be considered @garrettlondon1

garrettlondon1 commented 3 weeks ago

@iphdav I think you are focused exclusively on Blazor, but consider a RazorComponentResult MinimalAPI scenario.

SubComponents initializing has nothing to do with this.

If you want to use [StreamRendering], that's awesome..

But, for people who don't use Blazor.web at all, and are using MinimalAPI with RazorComponentResult, there is absolutely no reason that their code should be littered with null checks

iphdav commented 3 weeks ago

But @garrettlondon1 the same thing applies. If I have a minimal API returning a RazorComponentResult, I want the generated HTML to be returned with as low latency as possible.

The component I am rendering might use a bunch of sub-components that each have their own async logic in their own OnInitializedAsync methods that I want to kick off immediately. I do not want a 10 second delay for the outer component, only to then have another delay due to some other async stuff in the inner components.

I do have experience with this @garrettlondon1 including your scenario, where I am doing this exact thing for rendering email templates - returning RazorComponentResult via Minimal API, so that exact scenario. What you are suggesting would radically slow down the overall response time (increase latency).

The team should not consider anything like this.

I understand you have been playing with HTMX and Blazor, but you are clearly not dealing with deeply nested components and/or doing much async stuff or you would have appreciate what I am describing.

garrettlondon1 commented 3 weeks ago

@iphdav I completely agree with you that sub-components should not wait for their parent component to render.

In my opinion:

Once all OnInitializedAsync calls are finished, then view is rendered.

In a non-streaming scenario, this has absolutely no impact on performance.

iphdav commented 3 weeks ago

@garrettlondon1 Please have a play and you should very quickly figure out the problem. I have lots and lots of code that such a change would harm.

Here is an InnerComponent.razor

<div>Name is @Name</div>
@code {
  [Parameter] public string Name {get; set;}
  protected override async Task OnInitializedAsync()
  {
    await Task.Delay(10000); // some long network request
    // some work with the result
  }
}

Test.razor:

@foreach (var name in names)
{
  <InnerComponent Name="@name" />
}
@code {
  List<string> names = new();
  protected override async Task OnInitializedAsync()
  {
    names = ["Fred", "Mary"];
    await Task.Delay(10000); // some long network request
    // some work with the response
  }
}

Study this @garrettlondon1 and try to understand what you are suggesting would take 20 seconds instead of the 10 seconds it would take today. Now try to imagine a large system, like the kind of system I tend to build with blazor at my firm. The change you are suggesting would do a whole lot of harm.

Please think this through and understand that, in a large number of cases, you need to do the render to even know what components you will be rendering as children, and kick off their OnInitializeAsync logic.

garrettlondon1 commented 3 weeks ago

@iphdav correct me if I am wrong please:

With the way non-stream rendered blazor currently works your example above would finish in 20 seconds

Why:

  1. Initial render of the view : names is empty
  2. Test.razor OnInitializedAsync non stream rendered would finish 10 second call then render the View with names loop
  3. InnerComponent OnInitializedAsync starts
  4. Rest of component renders - 20 seconds
iphdav commented 3 weeks ago

No, I will explain the component model a bit further.

First, the initial render will happen on the first await. All the code in OnInitializeAsync will run up until that first await, then a render will occur. So names = ["Fred", "Mary"]; has run by this time. At that point the main component will render and thus the two sub-components will be initialized. Their OnInitializeAsync methods will thus kick off immediately.

In summary the total will be about 10 seconds, not 20 like you suggested, and I think you are not clear on the model. By default with OnInitializeAsync, the first render is when you hit the first await. Then there is a second render when the method finishes. You can add additional renders by calling StateHasChanged() and awaiting something else that is asynchronous, even just an await Task.Yield;.

Thus with your OnInitializeAsync method, you should setup as much as you can, as early as you can, then do a render to kick off the sub-components (child components).

garrettlondon1 commented 3 weeks ago

That makes sense, I want to be clear, this also happens in non-stream rendered scenarios?

I am envisioning non-stream rendered scenarios behave exactly like Web API, request response. I am confused how in a non-streamrendered response, the view can be rerendered before the OnInitialized.

Let me try debugging it for a bit and seeing what happens.. I don't pretend to have all the answers regarding this, but I would expect it to behave like Web API. Request comes into the OnInitializedAsync, and view is returned.. View should not be invoked until processing is completed.

How this interacts with Blazor, rendering, etc.. I have a lot to debug/learn clearly

iphdav commented 3 weeks ago

@garrettlondon1 When you have a spare moment, do check the blazor source code, at least for the ComponentBase.cs as it is really not that complex. I found spending a couple of hours looking at that file was better than 20 hours spent on learn.microsoft.com or watching youtube videos. ;-)

garrettlondon1 commented 3 weeks ago

I completely agree with you that child components should not suffer from this

iphdav commented 3 weeks ago

@garrettlondon1 Search for "ComponentBase.cs github blazor"

Look around line 274, and you can see Blazor has var task = OnInitializedAsync(); and it is not awaited. If you think about what is happening here, all the code will run up until the first await (inside an OnInitializedAsync method) and then that Task will be returned.

You can then see in the source code, Microsoft check if the Task is not completed (and not cancelled) and if so immediately queues a render by calling StateHasChanged() There is obviously another render when the task completed.

That is the reason a render occurs on the first await.

As an aside, there is lots of scope for the community to experiment with our own base classes with radically different logic.

garrettlondon1 commented 3 weeks ago

In reality the easiest way to solve for this without creating a base class is to use Minimal APIs and put all the processing inside of the endpoint and not the OnInitializedAsync.

Then, it's just an ugly passing of a ViewModel parameter to the RazorComponentResult and you don't have to worry about anything in the razor view.

I wanted to do it natively inside of the component itself since it will be cleaner.. but now I realize ComponentBase is the issue

javiercn commented 3 weeks ago

@garrettlondon1 thanks for contacting us.

This behavior is baked into ComponentBase and we aren't likely to special case it for streaming rendering. It would be a big breaking change and not something we think it's even correct.

You can provide your own base class as you point out in your last comment https://github.com/dotnet/aspnetcore/issues/57386#issuecomment-2295331008 and implement your own lifecycle to render whenever you need.

dotnet-policy-service[bot] commented 3 weeks ago

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.