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.37k stars 9.99k forks source link

Blazor Component Cannot Be Updated By Parent #22180

Closed mysteryx93 closed 4 years ago

mysteryx93 commented 4 years ago

Using the following code, I'm binding model properties for display in Blazor.

Updating Coupon.Message and Coupon.Error does display it as expected.

However, changing Products[0].Price doesn't update the display. Only difference I can see is that it's part of a collection.

If it's a similar issue as in WPF, then I tried replacing List with ObservableCollection but that's not helping.

Why is the update working for direct properties but not when part of collections?

I'm also curious -- how is it keeping track of property changes? In WPF, there is INotifyPropertyChanged and ObservableCollection, and none of that in Blazor yet it's still working. What's the magic behind it? (and why isn't that magic being used in WPF)

<p class="blue">Order Summary</p>
<div class="grid gridheader gridcolProduct">Product</div>
<div class="grid gridheader gridcolQuantity">Quantity</div>
<div class="grid gridheader gridcolPrice">Price</div>
<div class="grid gridheader gridcolTotal">Total</div>
<div class="clear"></div>
@foreach (var item in Products)
{
    <div class="grid gridcolProduct">@(item.DisplayName ?? item.Name)</div>
        @if (item.AllowEditQuantity)
        {
            <RadzenNumeric @bind-Value="item.Quantity" Min="1" Max="100" Style="width:100%" />
        }
        else
        {
            @item.Quantity
        }
    <div class="grid gridcolPrice">
        @if (item.StandardPrice.HasValue && item.StandardPrice != item.Price)
        {
            <s>$@item.StandardPrice</s>
        }
        &nbsp;$@item.Price
    </div>
    <div class="grid gridcolTotal">$@(item.Price * item.Quantity)</div>
    <div class="clear"></div>
}
<div class="grid gridtotaltext">Grand Total: </div>
<div class="grid gridtotal">$@Total USD</div>
<div class="clear">&nbsp;</div>

<EditForm Model="@Coupon" OnValidSubmit="CouponSubmitAsync" class="form">
    <div style="margin-bottom: 1rem" class="row">
        <Field For="@(() => Coupon.CouponCode)" Width="4" Required="true">
            <RadzenTextBox @bind-Value="@Coupon.CouponCode" class="form-control" id="CouponCode" />
        </Field>
        <RadzenButton ButtonType="ButtonType.Submit" Text="Apply" ButtonStyle="ButtonStyle.Light" />
        <div>@Coupon.Message</div>
        <div class="alert">@Coupon.Error</div>
    </div>
</EditForm>
<p>&nbsp;</p>

@code {
public CouponModel Coupon { get; set; } = new CouponModel();
public class CouponModel
{
    public string? CouponCode { get; set; }
    public string? Message { get; set; }
    public string? Error { get; set; }
}

[Parameter]
[SuppressMessage("Usage", "CA2227:Collection properties should be read only", Justification = "Reviewed: Allow set via attribute")]
public List<OrderFormProduct> Products { get; set; } = new List<OrderFormProduct>();

.....
}

The submit button calls this code which "should" edit the displayed quantity.

public void ValidateCoupon(ValidatingCouponEventArgs e)
{
    if (e.CouponCode == "valid")
    {
        e.Products[0].Price = 147;
        e.Message = "10% discount!";
        e.IsValid = true;
    }
    else
    {
        e.Products[0].Price = 296;
    }
}

Curiously, if I edit the quantity via the RadzenNumeric, both the total and grand total get updated in the browser! Until I click "submit coupon" and it reverts back to quantity 1 and the original price.

mysteryx93 commented 4 years ago

I've done more tests on this. If the button click sets the properties directly, then everything works fine, including the items in the list.

However, this fails to update binding

protected async Task CouponSubmitAsync()
{
    var e = new ValidatingCouponEventArgs(Products, Coupon.CouponCode ?? "");
    await ValidatingCoupon.InvokeAsync(e).ConfigureAwait(false);
}

With this in the page using the Component

public void ValidateCoupon(ValidatingCouponEventArgs e)
{
    if (e.CouponCode == "valid")
    {
        e.Products[0].Price = 147;
        e.Message = "10% discount!";
        e.IsValid = true;
    }
    else
    {
        e.Products[0].Price = 296;
    }
}

But if I instead write this, it works

protected async Task CouponSubmitAsync()
{
    var e = new ValidatingCouponEventArgs(Products, Coupon.CouponCode ?? "");
    if (e.CouponCode == "valid")
    {
        e.Products[0].Price = 147;
        e.Message = "10% discount!";
        e.IsValid = true;
    }
    else
    {
        e.Products[0].Price = 296;
    }
}

Which means: binding only works if updated from within the class. How can I work around this limitation?

mysteryx93 commented 4 years ago

I'm getting close!

In the page calling the component...

<OrderForm ValidatingCoupon="ValidateCoupon" 
       Product="@(new OrderFormProduct() {
                    Name = "Energy Tune Up",
                    DisplayName = "Soul Alignment Reading",
                    StandardPrice = 497,
                    Price = 296,
                    AllowEditQuantity = true,
                    AllowRemove = true
                  })" />

If I remove the Product parameter, then it's behaving as expected. This means that the Product property is being set and reset on every action!

So now the question is: how to set the initial value and avoid resetting it?

Alright got it. I must create a field and bind it to the field.

<OrderForm ValidatingCoupon="ValidateCoupon" 
           Product="@product" />

@code {
    public OrderFormProduct product = new OrderFormProduct()
    {
        Name = "Energy Tune Up",
        DisplayName = "Soul Alignment Reading",
        StandardPrice = 497,
        Price = 296,
        AllowEditQuantity = true,
        AllowRemove = true
    };

    public void ValidateCoupon(ValidatingCouponEventArgs e)
    {
        if (e.CouponCode == "valid")
        {
            e.Products[0].Price = 147;
            e.Message = "10% discount!";
            e.IsValid = true;
        }
        else
        {
            e.Products[0].Price = 296;
        }
    }
}

This works. I feel there are few things in here that require a few explanations and that aren't clear in the documentation.

mysteryx93 commented 4 years ago

To expand on this, if the object requires initialization using other members, what's the right way to initialize it?

This won't work because field initializer can't access other members.

<OrderForm Product="@product" />

@code {
    private OrderFormProduct product = new OrderFormProduct("Product1", Price);
    public decimal Price => Params.GetInt32("p", 1, 10000) ?? 497;
}

What's the right way to do it, overriding OnInitialize, or creating a lazy-loading property that initializes when first called?

public OrderFormProduct Product => _product ?? (_product = new OrderFormProduct("God Money Masterclass", Price));
private OrderFormProduct? _product;
mysteryx93 commented 4 years ago

I've read and re-read this section multiple times: Don't create components that write to their own parameter properties

That definitely definitely should be improved as it's not clear at all.

I've changed the code so that the child component reads the parameter only during OnInitialized, but there some problems with that

This section of documentation definitely needs clarification.

mkArtakMSFT commented 4 years ago

@guardrex some feedback here for the docs.

ghost commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

guardrex commented 4 years ago

Thanks @mysteryx93 ... I'm just seeing this for the first time. To reach us on the docs side, open a doc issue with the This page feedback button+form at the bottom of any topic. That will add metadata linking the topic to a docs issue on the docs repo and pings me immediately.

We have an issue for that guidance at https://github.com/dotnet/AspNetCore.Docs/issues/18144. I'll work it further from there. I should be getting to that fairly soon. I'll look at what you posted :point_up: when I work the issue and see if there's general guidance that we can place into the topic. Note that addressing your scenario specifically depends on how generally applicable the scenario is.

Using a field only prevents overwriting under the narrow conditions explained in the text ...

If that's not a scenario for a component, then the dev doesn't need to sweat that guidance AFAIK. So, we probably need to change the way it's described. Instead of implying/saying 'NEVER do this,' it should probably lean more toward 'under these circumstances and to avoid this behavior, do this.'

@oiBio's full Expander example was placed there to help understand the scenario. @mysteryx93, if you haven't already tried it, I recommend that you run the Expander component examples in a test app to see the behavior of each. I think we should retain the example and probably place it into the common Blazor sample app later so that devs can experience the scenario more directly/easily.

guardrex commented 4 years ago

@mysteryx93 ... I've finally 😅 reached that pair of component param and binding doc issues. I'm working up a draft PR now at https://github.com/dotnet/AspNetCore.Docs/pull/19557 that I hope will fix the confusion with that section on param state preservation:

Fortunately, the section itself seems ok. The conditions that define the problem are clear. At this time, I can't say if there are any gotchas or edge cases that we need to call out.

Among other things, the PR seeks to re-title that section and better describe the scenario near the cross-link. The conditions of losing state there are as I described in my earlier remark and as the existing section states: RenderFragment child content with StateHasChanged called in the parent.

@mkArtakMSFT ... The in-process doc updates should resolve the most pressing and obvious problems. However, I haven't tried to grok @mysteryx93's use case ☝️, which is complex, to see what else might be needed. If I were to do so, I think I would need a working repro project put up on GH that I could take a look at with a clear description of other problem(s) that I haven't addressed with the upcoming PR.

guardrex commented 4 years ago

@mkArtakMSFT Not sure if there's more to resolve here, but I can confirm that the component param language updates have been merged into the docs with Steve's blessing. I don't have further open docs issue(s) to address on this subject.

captainsafia commented 4 years ago

Closing this for now in favor of direct feedback on the aspnetcore.docs repo. There's not a clear ask for changes in Blazor here. Please open a new issue if there is further feedback.