Aspnetcore: Blazor Two Way Binding Error

[simonziegler]

Describe the bug

A component with a parameter called say Value manages 2 way binding with an accompanying ValueChanged event handler. If (i) this event handler is called from within the setter for Value and (ii) the component attaches to a cascading value, the component's ability to set the value is neutralized by the value bouncing back to its original state.

For what it's worth I believe that I have seen repetitive 2 way binding bounce in the past but cannot reproduce that.

To Reproduce

Please fork https://github.com/simonziegler/TwoWayBindingDemo! The demonstration is in the index page. You will find two components each of which is referenced both within and outside a cascading value. The one that fails has its result shown in red. At the bottom of the page you'll see a list of the bound value changes captured by the parent - you'll see a bounce on the offending version.

Exceptions (if any)

n/a

Further technical details

• ASP.NET Core version: both .NET 3.1 and .NET 5
• Include the output of dotnet --info

.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.7.20366.6
 Commit:    0684df3a5b

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19041
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100-preview.7.20366.6\

Host (useful for support):
  Version: 5.0.0-preview.7.20364.11
  Commit:  53976d38b1

.NET SDKs installed:
  3.1.201 [C:\Program Files\dotnet\sdk]
  3.1.302 [C:\Program Files\dotnet\sdk]
  3.1.400-preview-015203 [C:\Program Files\dotnet\sdk]
  5.0.100-preview.7.20366.6 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.20 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0-preview.7.20365.19 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.20 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0-preview.7.20364.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.1.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.6 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.0-preview.7.20366.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download

• The IDE (VS / VS Code/ VS4Mac) you're running on, and it's version
  

[simonziegler]

Update. @SQL-MisterMagoo has, as always, given an excellent insight here. I have added a "Component 1a" using his pattern of calling ValueChanged from a backing value setter rather than the public parameter Value's setter. This pattern works.

This raises the question of whether I have found a bug or perhaps just something that needs to be covered with an advisory note in your two way binding documentation.

Also @stefanloerwald has pointed out that changing the cascading value to this 👇 with IsFixed removes the bounce, although fixed cascading values aren't always desirable.

<CascadingValue Value="@cv" IsFixed="true">
    <Component1 @bind-Value="@WithCascadingValue1" />
    <p style="color: red">Value: @WithCascadingValue1 - this version fails.</p>

    <Component1a @bind-Value="@WithCascadingValue1a" />
    <p>Value: @WithCascadingValue1a</p>

    <Component2 @bind-Value="@WithCascadingValue2" />
    <p>Value: @WithCascadingValue2</p>
</CascadingValue>

[stefanloerwald]

The way I see it, the loop back to setting values in the component is unavoidable. However, it shouldn't reset any values. The order of things should be

1. change the value of the property
2. invoke ValueChanged with the new value
3. Parent component receives the event callback, and updates its value
4. Parent component re-renders, as state has changed
5. This implicitly causes the child component to get updated values. At this point, the child should receive the changed value, not the old value (this seems to me where the bug is)
6. The child component receives the new value. Here it is either responsibility of the framework to not change values that didn't change (that isn't yet the case, right? Probably not a universally correct thing to do?) or the programmers responsibility to have the safeguard if (backing_field == value) { return; } in the setter of the property.

In conclusion, the following code should never fail:

private T _value;
[Parameter] public T Value
{
   get => _value;
   set
   {
      if (value == _value) { return; }
      _value = value;
      ValueChanged.InvokeAsync(_value);
   }
}

This can't result in an infinite loop, because the condition in the setter breaks it, under the assumption that the value that is bound in the parent component is updated to the new value before the re-render and the re-setting of the parameters happens.

[simonziegler]

I think I come back to the notion that I've noticed "bounciness" in two way binding.

[SQL-MisterMagoo]

Maybe the key to this is in this line ?
"...under the _assumption_ that the value that is bound in the parent component is updated to the new value before the re-render and the re-setting of the parameters happens"

I mean I don't know for sure, but I do know it felt really bad to be doing it that way - and changing it the way I did superficially confirmed it as a reasonable concern - because it works the other way.

I would also note that I would never "double-bind" like this example (where the incoming parameter is then bound inside the component) anyway.

I always bind the backing field to the html element in my component - as in my example - and so I have never had this problem.

So - TLDR; - The reason I didn't personally add my example here is because I am just another user and didn't want to sidetrack this issue with what could be considered a workaround.

[stefanloerwald]

I've played around with the code and added some Console.WriteLine. This reveals the following outputs:

For Component1 without cascading value:

Child: Value set invoked with value = 0
Child: OnParametersSet
Child: OnAfterRender
Child: Value set invoked with value = 1
Child: Value setting. Old value = 0
Child: Value set. New value = 1
Parent: Value updated to 1
Child: ValueChanged invoked
Child: Value changed to 1
Child: Value set invoked with value = 1
Child: OnParametersSet
Child: OnAfterRender

And with cascading value it becomes

Child: Value set invoked with value = 0
Child: OnParametersSet
Child: OnAfterRender
Child: Value set invoked with value = 1
Child: Value setting. Old value = 0
Child: Value set. New value = 1
Parent: Value updated to 1
Child: ValueChanged invoked
Child: Value changed to 1
Child: Value set invoked with value = 0
Child: Value setting. Old value = 1
Child: Value set. New value = 0
Parent: Value updated to 0
Child: ValueChanged invoked
Child: OnParametersSet
Child: Value set invoked with value = 0
Child: OnParametersSet
Child: Value set invoked with value = 0
Child: OnParametersSet
Child: Value set invoked with value = 0
Child: OnParametersSet
Child: OnAfterRender
Child: OnAfterRender
Child: OnAfterRender

The diff is therefore:

Child: Value set invoked with value = 0
Child: OnParametersSet
Child: OnAfterRender
Child: Value set invoked with value = 1
Child: Value setting. Old value = 0
Child: Value set. New value = 1
Parent: Value updated to 1
Child: ValueChanged invoked
Child: Value changed to 1
- Child: Value set invoked with value = 1
+ Child: Value set invoked with value = 0
+ Child: Value setting. Old value = 1
+ Child: Value set. New value = 0
+ Parent: Value updated to 0
+ Child: ValueChanged invoked
Child: OnParametersSet
+ Child: Value set invoked with value = 0
+ Child: OnParametersSet
+ Child: Value set invoked with value = 0
+ Child: OnParametersSet
+ Child: Value set invoked with value = 0
+ Child: OnParametersSet
Child: OnAfterRender
+ Child: OnAfterRender
+ Child: OnAfterRender

This to me shows the bug quite clearly: The parent receives the correct value, yet the child component is subsequently updated to the original value again.

[msftbot[bot]]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[simonziegler]

@mkArtakMSFT, with respect, doesn't this warrant some investigation rather than delay to a future sprint date - whenever that might be?

[MarkStega]

I too have experienced this issue and have spent endless hours trying to understand. If this isn't considered a bug it will require some excellent documentation as the behavior is quite contrary to what one would expect. I hardly expect this sequence:

• if I change a parameter value in a component

• signal a value change to the parent

• then the parent first sets the parameter to the old value

[simonziegler]

@stefanloerwald added a separate page (from the menu) with his logging output and this is merged into the repo. Please run the solution without IIS Express and you'll see his logging results in the command window that shows the results of dotnet run

[simonziegler]

Watching the community stand up just now @danroth27 said to point out what needs attention in issues. This is one!

[simonziegler]

Reading #18919 it seems to me that these issues are linked. @tragetaschen mentions the double rendering from StateHasChanged with first the old state and then the new one. It feels that this could be the cause if this issue.

[SteveSandersonMS]

The core issue here is that components shouldn't overwrite their own incoming parameter value properties. If they do that, then those changes can themselves always be overwritten when the parent re-renders. This is described in docs at https://docs.microsoft.com/en-us/aspnet/core/blazor/components/?view=aspnetcore-3.1#overwritten-parameters

There are some cases where, when a parent re-renders, the framework's optimizer knows it can skip re-rendering the child (i.e., it doesn't need to call SetParametersAsync on the child). In those cases the child's parameters won't be overwritten (which is why it doesn't happen in all cases in the repro project). But you shouldn't rely on this because it depends on a whole range of things, and it's too risky to rely on correctly anticipating this. Instead you should simply avoid having your component overwrite its own incoming [Parameter] property values, and store any internally mutable state on separate private fields/properties. Then things will always behave as you want and expect.

[stefanloerwald]

I don't think this analysis is accurate, @SteveSandersonMS. As you can see from an earlier comment https://github.com/dotnet/aspnetcore/issues/24599#issuecomment-669866377, the value in the parent gets updated before the re-render, therefore when the re-render happens, it should only ever see the updated value.

[SteveSandersonMS]

I see, thanks for clarifying! This will require more investigation then. I'll reopen. It may well be that this is a behavior we can't change in 5.0 because we're so close to the final ship date, but I'll try to give a clearer explanation of what's going on.

[SteveSandersonMS]

OK, I've looked in more detail and see what's going on. There's an explanation below, but before getting to that, I'll restate my claim that the core problem is the child component overwriting its own [Parameter] property value. By avoiding doing that, you can avoid this problem.

In this instance, and in others, Blazor relies on parent-to-child parameter assignment being safe (not overwriting info you want to keep) and not having side-effects such as triggering more renders (because then you could be in an infinite loop). We should probably strengthen the guidance in docs not just to say "don't write to your own parameters" and expand it to caution against having side-effects in such property setters. Blazor's use of C# properties to represent the communication channel from parent to child components is pretty convenient for developers and looks good in source code, but the capacity for side-effects is problematic. We already have a compile-time analyzer that warns if you don't have the right accessibility modifiers on [Parameter] properties - maybe we should extend it to give warnings/errors if the parameter is anything other than a simple { get; set; } auto property.

So in this case the solution is pretty simple: replace Component1's Value property with a simple { get; set; } one, and the instead of trying to write to it, have Component1 respond to button clicks by triggering ValueChanged. Then you don't have any recursive render cycle, and no data overwriting occurs.

    private void DecrementValue()
    {
        //Value--; <-- Don't do this

        // Do this instead:
        ValueChanged.InvokeAsync(Value - 1);
    }

Behind the scenes

The problem you observe is because, when there's a <CascadingValue>, there are two different sources of parameters for the child:

• The <CascadingValue> component
• The parent component

When a rendering cycle occurs, one of these has to supply values before the other, and in your case it happens to be the <CascadingValue>. For consistency, when CascadingValue supplies updated parameters to a subscriber, the subscribee receives not only the cascaded parameter value but also a snapshot of whatever previous set of parameters it received. This is so that you don't have to worry about implementing any special-case logic to allow for parameters being missing. In the event that you're modifying a parameter value during the same cycle as a cascaded value is propagating, the parameter snapshot will still contain the previous value. Normally this causes no problems because, as long as setting the parameter values is not side-effecting, the renderer will continue and also supply the updated values from the parent that's re-rendering, and so you end up with a consistent and correct set of final values. The problem only occurs because setting the parameter values in the repro case does have side-effects which include triggering another re-render on the parent. The solution therefore is to leave the parameter communication channel alone (i.e., don't write to the properties, nor put in set logic that has side-effects).

I hope all this makes sense. If you think I'm still missing something, please let me know. The actions I plan to take on this are:

• Improve docs to clarify the requirements around parameter passing
• Add a backlog item to consider a compile-time analyzer to give warnings if a [Parameter] property isn't just { get; set; } or if it's written to from inside the component

[simonziegler]

Thanks @SteveSandersonMS. This requires quite a re-think on our part, especially for Material.Blazor. We'll spend some time working through this and may report back if we get any issues. Of course we may also report a success back too!