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

Support components mutating SupplyParameterFromForm data #49442

Closed danroth27 closed 1 year ago

danroth27 commented 1 year ago

I have a form that raises an event callback on a valid submit. When I invoke the callback, the form no longer resets. If I don't invoke a callback it resets just fine.

Test.razor

<EditForm method="post" Model="Review" OnValidSubmit="OnValidSubmit">
    <InputText @bind-Value="Review.Text" />
    <button type="submit">Submit</button>
</EditForm>
<p>Text: @Review.Text</p>
<p>Message: @message</p>

@code {
    string message = "";

    [SupplyParameterFromForm]
    public Review Review { get; set; } = new();

    [Parameter]
    public EventCallback<Review> OnSubmitReview { get; set; }

    async Task OnValidSubmit()
    {
        message = Review.Text;
        await OnSubmitReview.InvokeAsync(Review); // The form text field doesn't reset if this is here
        Review = new();
    }
}

Index.razor

<Test OnSubmitReview="OnSubmitReview" />

@code {
    void OnSubmitReview(Review review)
    {
        Console.WriteLine("Form submitted");
    }
}

Review.cs*

public class Review
{
    public string Text { get; set; } = "";
}

Repro steps:

Expected result: Message updates and form is reset after submitting it Actual result: Message updates but the form doesn't get reset. Something populates the Review property again with the submitted value.

SteveSandersonMS commented 1 year ago

This code is violating the rule of not overwriting your own parameter properties. Doing so is unwise because the system also writes to those parameter properties whenever your component re-renders, thereby overwriting your changes. For example in this case, calling OnSubmitReview causes the parent to-rerender, which re-renders its child, re-supplying its parameters, overwriting your change to them.

We have documented this at https://learn.microsoft.com/en-us/aspnet/core/blazor/components/overwriting-parameters?view=aspnetcore-7.0 but I certainly see that relying on docs alone is insufficient. The compiler is happy to let people do this so we shouldn't assume people understand why it will get them into trouble. Even if people do know about this, which I think @danroth27 does, it's so easy to forget as this proves.

We have a proposal to add an analyzer for this (https://github.com/dotnet/aspnetcore/issues/44690) but it didn't make the cut for .NET 8. Perhaps we should reprioritize this for .NET 9.

danroth27 commented 1 year ago

It seems like the form model and the model bound parameter need to be the same. Otherwise model binding doesn't work. The form data is based on the model name. Given that is the case, how then should you correctly clear the state of the form?

danroth27 commented 1 year ago

After discussion, the proposal is to change the behavior of SupplyParameterFromForm so that it doesn't get reapplied. While this is different than how cascading values normally work, we think this makes sense because the form data isn't going to change.