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

Components render too many times with SetParametersAndRender #1119

Closed
Jcparkyn opened this issue Jun 19, 2023 · 25 comments · Fixed by #1125
Closed

Components render too many times with SetParametersAndRender #1119

Jcparkyn opened this issue Jun 19, 2023 · 25 comments · Fixed by #1125
Labels
bug Something isn't working help wanted Extra attention is needed input needed When an issue requires input or suggestions investigate This issue require further investigation before closing.

Comments

@Jcparkyn
Copy link
Contributor

Jcparkyn commented Jun 19, 2023

In certain cases, calls to StateHasChanged behave differently in BUnit than in regular Blazor WASM (I haven't tested server-side, but I'd expect it to have the same problem). This means that components in BUnit render too many times, and RenderCount values in BUnit are often higher than they should actually be. I have observed this when using SetParametersAndRender, but I'm not sure if there are other scenarios where this happens.

This is a problem in my case, because I care a lot about the number of renders, but I have no way to reliably assert the RenderCount in tests.

Example:
Testing this component:

<p>Hello world!</p>

@{
    Console.WriteLine("Rendering RenderCountDemo");
}

@code {

    [Parameter]
    public int X { get; set; }

    protected override void OnParametersSet()
    {
        base.OnParametersSet();
        StateHasChanged();
        StateHasChanged();
        StateHasChanged();
    }
}

With this test:

[Fact]
public void Should_have_correct_RenderCount()
{
    var cut = Render<RenderCountDemo>(@<RenderCountDemo X="1" />);
    cut.SetParametersAndRender(); // This should only trigger one additional render.
    cut.RenderCount.Should().Be(2);
}

Results in this output:

Expected cut.RenderCount to be 2, but found 5

Expected behavior:

The test passes. Using the same component in a Blazor WASM app results in one render each time the parameter changes, but in BUnit I get three (edit: four). This is not just a problem with the "RenderCount" property, the actual render method is being called four times as well (confirmed with a breakpoint).

Version info:

  • bUnit version: 1.20.8
  • .NET Runtime and Blazor version: .NET 6.0
  • OS type and version: Windows 10

Additional context:

I am obviously not calling StateHasChanged() multiple times in succession like the example above, but there are lots of dependencies etc. which can result in redundant calls to StateHasChanged. In a normal Blazor app, the calls would get "batched" into a single render, so long as they occur synchronously.

@egil
Copy link
Member

egil commented Jun 19, 2023

Hi @Jcparkyn,

Thanks for reaching out. We have tried to stabilize bUnit the last few months related to blocking the renderer when triggering asynchronous renders. A side effect of that may be what you are observing here. I will try to replicate and see what options we have.

@egil egil added the investigate This issue require further investigation before closing. label Jun 19, 2023
@egil
Copy link
Member

egil commented Jun 19, 2023

@linkdotnet this is a case we should also support in V2 code path.

@linkdotnet
Copy link
Collaborator

linkdotnet commented Jun 19, 2023

By any chance can you elaborate how you counted the render cycles in the WebAssemblyRenderer?

@Jcparkyn
Copy link
Contributor Author

By any chance can you elaborate how you counted the render cycles in the WebAssemblyRenderer?

I counted the render cycles by using the Console.WriteLine inside the render function (and by counting how many times a breakpoint in the render function was hit). In my case, one line was written to the console each time the component parameter changed.

Here's a minimal repro which can be pasted into the "Empty Blazor Webassembly" template:

RenderCountDemo.razor:

@{
    Console.WriteLine("Rendering RenderCountDemo");
}

X: @X

@code {

    [Parameter]
    public int X { get; set; }

    protected override void OnParametersSet()
    {
        base.OnParametersSet();
        StateHasChanged();
        StateHasChanged();
        StateHasChanged();
    }
}

Index.razor:

@page "/"

<button @onclick="() => count++">Click me</button>

<RenderCountDemo X="count" />

@code {
    int count = 0;
}

I should also mention that the problem occurs in BUnit 1.12.6 as well (that's what I was originally using), so I don't think it was a recent regression.

@egil
Copy link
Member

egil commented Jun 20, 2023

I've tried this locally, and I am not sure what our options are, if any.

My test looked like this:

public class MultipleStateHasChangedInOnParametersSet : ComponentBase
{
  protected override void OnParametersSet()
  {
    base.OnParametersSet();
    StateHasChanged();
    StateHasChanged();
    StateHasChanged();
  }
}

[Fact]
public void Multiple_calls_to_StateHasChanged_from_OnParametersSet()
{
  var cut = RenderComponent<MultipleStateHasChangedInOnParametersSet>();
  cut.RenderCount.ShouldBe(1);
  
  cut.SetParametersAndRender();
  cut.RenderCount.ShouldBe(2); // fails, cut.RenderCount == 5.
}

Some notes:

  • During the initial render, e.g. triggered by RenderComponent, the OnParametersSet method is called, and all the StateHasChanged calls are batched up as you say is the normal experience.

  • OnParameterSet is also called after cut.SetParametersAndRender() is triggered, however, here each StateHasChanged call runs synchronously through a full render cycle. before returning, and the next one does the same.

    I think this happens because when the first StateHasChanged call is triggered, there is no other ongoing render happening, which the StateHasChanged calls can be batched up with, so the Blazor runtimes prefer to just run everything synchronously if it can. This means that one StateHasChanged call completes after the other, resulting in a full render cycle.

Interestingly, if we trigger an update to parameters in the following way:

@* TriggerChildContentRerenderViaClick.razor *@
<button @onclick="() => counter++">Count = @counter</button>
<MultipleStateHasChangedInOnParametersSet Value="counter" />
@code {
    int counter;
}
public class MultipleStateHasChangedInOnParametersSet : ComponentBase
{
  [Parameter]
  public int Value { get; set; }
  
  protected override void OnParametersSet()
  {
    base.OnParametersSet();
    StateHasChanged();
    StateHasChanged();
    StateHasChanged();
  }
}

[Fact]
public void Multiple_calls_to_StateHasChanged_from_OnParametersSet_with_event_dispatch_render_trigger()
{
  var cut = RenderComponent<TriggerChildContentRerenderViaClick>();
  var child = cut.FindComponent<MultipleStateHasChangedInOnParametersSet>();
  
  child.RenderCount.ShouldBe(1);
  
  cut.Find("button").Click();
  
  child.RenderCount.ShouldBe(2); // this assertion passes
}

The expected number of renders does occur.

So, I think we need to investigate if we can change our cut.Render and cut.SetParametersAndRender methods to behave similarly to what happens when a parent component passes down new parameters to a child component as part of an existing render cycle.

@egil egil added bug Something isn't working help wanted Extra attention is needed input needed When an issue requires input or suggestions labels Jun 20, 2023
@egil
Copy link
Member

egil commented Jun 20, 2023

Pushed a branch with my (failing tests) for this issue to https://github.com/bUnit-dev/bUnit/tree/bug/1119-Components-render-too-many-times-with-SetParametersAndRender if somebody wants to take a look. Otherwise, I will see what I can do later during the summer.

@Jcparkyn
Copy link
Contributor Author

Thanks @egil, I think I'll be able to get by with your workaround (or something similar) for now. I'll update this thread if I find anything useful. Here's another workaround that seems to work:

@* RenderCountWrapper.razor *@
@ChildContent
@code {
    [Parameter] public RenderFragment? ChildContent { get; set; }
}
[Fact]
public void Should_have_correct_RenderCount() // passes
{
    var x = 10;
    var cut = Render<RenderCountWrapper>(@<RenderCountWrapper><RenderCountDemo X="x" /></RenderCountWrapper>);
    var component = cut.FindComponent<RenderCountDemo>();
    component.MarkupMatches("X=10");
    x = 20;
    cut.Render();
    component.RenderCount.Should().Be(2);
    component.MarkupMatches("X=20");
}

@linkdotnet
Copy link
Collaborator

Pushed a branch with my (failing tests) for this issue to https://github.com/bUnit-dev/bUnit/tree/bug/1119-Components-render-too-many-times-with-SetParametersAndRender if somebody wants to take a look. Otherwise, I will see what I can do later during the summer.

Interesting - will have a look in the upcoming days.

@linkdotnet
Copy link
Collaborator

Okay - here the reason for the different behavior between an initial render and a re-render.
The initial render is triggered in our TestRenderer. the Blazor Renderer has the following line:

if (!_isBatchInProgress)
{
    ProcessPendingRender();
}

In AddToRenderQueue. In the initial case _isBatchInProgress is true - therefore all StateHasChanged calls are packed together and only one render cycle happens.

In the second case (i.e. cut.Render() or cut.SetParameteresAndRender()). _isBatchInProgress is false as ProcessRenderQueue is never called. We are calling this directly:

var result = renderedComponent.InvokeAsync(() =>
			renderedComponent.Instance.SetParametersAsync(parameters));

This bypasses ProcessRenderQueue completely. RenderComponent does not do that.

@egil
Copy link
Member

egil commented Jun 21, 2023

That's what I thought, @linkdotnet. Do you see a way we can get around that? Using reflection to flip the _isBatchInProgress before calling renderedComponent.Instance.SetParametersAsync(parameters)?

@linkdotnet
Copy link
Collaborator

linkdotnet commented Jun 22, 2023

That's what I thought, @linkdotnet. Do you see a way we can get around that? Using reflection to flip the _isBatchInProgress before calling renderedComponent.Instance.SetParametersAsync(parameters)?

Well I still have to think a bit longer about that, but here is my current view:
Blazor renders in batchmode in @Jcparkyn example. But there is a big difference between the test and:

<p>Hello world!</p>

@{
    Console.WriteLine("Rendering RenderCountDemo");
}

@code {

    [Parameter]
    public int X { get; set; }

    protected override void OnParametersSet()
    {
        base.OnParametersSet();
        StateHasChanged();
        StateHasChanged();
        StateHasChanged();
    }
}

This component gets updated by the parents onclick event. And of course DispatchEventAsync (that is used when event handlers get invoked) will trigger ProcessRenderQueue. So if I would create the test where the parent is the cut and "just" click the button - I also would see only 2 render cycles for the component in question.

That brings me to the point of whether or not this should be "fixed". In a normal Blazor application, you don't have a way to just retrigger the Render method from the RenderHandle (when inheriting from ComponentBase). That we allow this, leads to side-effects. Just for the full picture StateHasChanged also doesn't force a render but more or less informs the renderer that the component would like to be re-rendered.

@linkdotnet
Copy link
Collaborator

@egil, the more I think about that, the less I am inclined to say we should change the behavior.
The test in question and what one sees in the browser are just too different. They do different things (see my last comment for a detailed explanation).

I am more in favor to restrict when one would call SetParametersAndRender and friends after the initial render (or remove it at all).

@Jcparkyn
Copy link
Contributor Author

@linkdotnet I'm not too familiar with the internals, but shouldn't there ideally still be some straightforward way to update the parameters of a CUT, that represents what would happen in a real app? Making a separate parent component per test is doable, but still a lot more work than having an interface like SetParametersAndRender().

Whether it's worth the time to implement is another question, of course.

@linkdotnet
Copy link
Collaborator

linkdotnet commented Jun 22, 2023

@linkdotnet I'm not too familiar with the internals, but shouldn't there ideally still be some straightforward way to update the parameters of a CUT, that represents what would happen in a real app? Making a separate parent component per test is doable, but still a lot more work than having an interface like SetParametersAndRender().

Whether it's worth the time to implement is another question, of course.

Fair point - my argument goes in the direction that the render count is an internal thing. As such, it can vary depending on what you are doing.

To your original request, what you observed in the browser and what you are doing in the test - they are just different things. And yes it is a bit unfortunate that it isn't visible to you. Functions like cut.SetParametersAndRender just don't have a real pendant in the browser hence the different behavior and are meant as a convenient way.

@egil
Copy link
Member

egil commented Jun 22, 2023

To your original request, what you observed in the browser and what you are doing in the test - they are just different things. And yes it is a bit unfortunate that it isn't visible to you. Functions like cut.SetParametersAndRender just don't have a real pendant in the browser hence the different behavior and are meant as a convenient way.

I do not disagree. That said, I think it would be nice if we can emulate what happens when a parent component passes new parameters to a child component via the cut.SetParametersAndRender() and cut.Render() methods.

@linkdotnet
Copy link
Collaborator

Yeah we could do that - probably, we have to investigate a bit deeper on what is going on here. I do feel not very comfortable setting a private flag via Reflection (but I don't see any alternative at the moment).

A bit off-topic but somewhat related I found some interesting bits in the Blazor code:

// Note: We should be careful to ensure that the framework never calls
// IComponent.SetParametersAsync directly elsewhere. We should only call it
// via ComponentState.SetDirectParameters (or NotifyCascadingValueChanged below).
// If we bypass this, the component won't receive the cascading parameters nor
// will it update its snapshot of direct parameters.

Source

@egil
Copy link
Member

egil commented Jun 22, 2023

Neat find. I'll play a little with reflection tonight and see what's possible.

For V2 and .net 8 the ComponentState has been - made public so we may have a less hacky way to the goal line

@linkdotnet
Copy link
Collaborator

Neat find. I'll play a little with reflection tonight and see what's possible.

For V2 and .net 8 the ComponentState has been - made public so we may have a less hacky way to the goal line

Nice - looking forward to what you are coming up.

For v2 - unfortunately almost all methods of ComponentState are internal.

@egil
Copy link
Member

egil commented Jun 22, 2023

The first attempt with just getting ComponentState and calling SetDirectParameters behave like before:

var result = renderedComponent.InvokeAsync(() =>
{
	var renderer = renderedComponent.Services.GetService<ITestRenderer>();
	var componentState = typeof(Renderer).GetMethod("GetRequiredComponentState", BindingFlags.Instance | BindingFlags.NonPublic).Invoke(renderer, new object[] { renderedComponent.ComponentId });
	var setDirectParametersMethod = componentState.GetType().GetMethod("SetDirectParameters", BindingFlags.Public | BindingFlags.Instance);
	setDirectParametersMethod.Invoke(componentState, new object[] { parameters });
});

This still triggers 5 render cycles.

@linkdotnet
Copy link
Collaborator

Yeah because those functions don't set the _isBatchInProgress variable. That is normally done in DispatchEventAsync.

@egil
Copy link
Member

egil commented Jun 22, 2023

Got it working. Only a POC, but this may be a path forward: d7a0a2a

I am not too afraid of using reflection for the older versions of Blazor, they are unlikely to change. For .NET 8 we may ping the team and ask if they can expose the functionality.

I am essentially doing:

  1. setting _isBatchInProgress to true
  2. using ComponentState.GetRequiredComponentState to set parameters`
  3. setting _isBatchInProgress to false
  4. calling Renderer.ProcessPendingRender

@linkdotnet
Copy link
Collaborator

Nice one. I'd argue that the majority of that code should live inside the TestRenderer itself - that would also remove the need of some of the reflection.

@linkdotnet
Copy link
Collaborator

Somewhat related: dotnet/aspnetcore#48980
I opened a ticket for our v2.

@egil
Copy link
Member

egil commented Jun 25, 2023

Hi @Jcparkyn, ill push a preview version of bunit out in a little while that you can try out. That should solve this issue. If not, please let us know.

@Jcparkyn
Copy link
Contributor Author

Thanks @egil, I tried out the latest preview and it's working perfectly, including on my real tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed input needed When an issue requires input or suggestions investigate This issue require further investigation before closing.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants