-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Blazor Two Way Binding Error #24599
Comments
Update. @SQL-MisterMagoo has, as always, given an excellent insight here. I have added a "Component 1a" using his pattern of calling 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 <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> |
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
In conclusion, the following code should never fail:
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. |
I think I come back to the notion that I've noticed "bounciness" in two way binding. |
Maybe the key to this is in this line ? 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. |
I've played around with the code and added some For Component1 without cascading value:
And with cascading value it becomes
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. |
Thanks for contacting us. |
@mkArtakMSFT, with respect, doesn't this warrant some investigation rather than delay to a future sprint date - whenever that might be? |
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:
|
@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 |
Watching the community stand up just now @danroth27 said to point out what needs attention in issues. This is one! |
Reading #18919 it seems to me that these issues are linked. @Tragetaschen mentions the double rendering from |
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 |
I don't think this analysis is accurate, @SteveSandersonMS. As you can see from an earlier comment #24599 (comment), 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. |
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. |
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 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 So in this case the solution is pretty simple: replace private void DecrementValue()
{
//Value--; <-- Don't do this
// Do this instead:
ValueChanged.InvokeAsync(Value - 1);
} Behind the scenesThe problem you observe is because, when there's a
When a rendering cycle occurs, one of these has to supply values before the other, and in your case it happens to be the 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:
|
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! |
…esign of events. Therefore, we have new major version
Describe the bug
A component with a parameter called say
Value
manages 2 way binding with an accompanyingValueChanged
event handler. If (i) this event handler is called from within the setter forValue
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
dotnet --info
The text was updated successfully, but these errors were encountered: