[Blazor] `Input*` - allow correct async handling of `ValueChanged` callbacks (incl. `@bind-Value:after`)

[hakenr]

Summary

In the current InputBase design, the binding to underlying input element is implemented by synchronous setter (CurrentValue_set). The asynchronous ValueChanged callback is called from the synchronous setter as "fire and forget" task (line 78): https://github.com/dotnet/aspnetcore/blob/905557eaca32f5ce9fd17a25e5187ceb7a75cf1c/src/Components/Web/src/Forms/InputBase.cs#L69-L82

Although there already have been less frequent scenarios where this turned out to be an issue (e.g. manual binding using Value, ValueChanged and ValueExpression parameters), starting with @bind-Value:after this will become much bigger pain. Subscribtion to the ValueChanged callback will become much easier and I expect the users will start using is much more (imagine continuous saving of values when filling the form).

I would like to start discussion on this topic, try to find a new design for InputBase and derived components and allow proper asynchronous handling of the ValueChanged callback (incl. @bind-Value:after variant).

Motivation and goals

Consider this sample code. The exception is "lost", neither blazor-error-ui, nor ErrorBoundary or browser/server-console will capture the exception:

@page "/"

<EditForm Model="model">
    <ErrorBoundary>
        <InputText @bind-Value="model" @bind-Value:after="DoSomethingAfterValueChanged" />
    </ErrorBoundary>
    @*<input type="text" @bind-value="model" @bind-value:after="DoSomethingAfterValueChanged" />*@
</EditForm>

@code {
    string model = String.Empty;

    private Task DoSomethingAfterValueChanged()
    {
        Console.WriteLine("This executes.");
        throw new InvalidOperationException("[1] This exception is lost in async-over-sync call from InputBase.CurrentValue_set.");
        // exception not logged in Console
        // exception not caught by ErrorBoundary
        // exception not caught by Blazor global error UI (the yellow strip of death :-D)
    }
}

(In opposite, the plain input HTML element will behave correctly.)

Risks

It is obvious, that the solution will cause major breaking changes in current Input* components as their protected API for inherited components would have to change significantly (the CurrentValue and CurrentValueAsString properties will have to be replaced with some asynchronous subsitutes). With such major impact, consider creating of new set of NewInput* components and keep the old ones as they are.

Examples

Give brief examples of possible developer experiences (e.g., code they would write).

Don't be deeply concerned with how it would be implemented yet. Your examples could even be from other technology stacks.

Detailed design

I would like to discuss the issue first with major stakeholders (e.g. @SteveSandersonMS, @javiercn, ...) and based on the approval that "this is something we want to solve" we can elaborate more on the detailed design. It is obvious that we have to handle the onchange callback from the input element in some asynchronous method rather than synchronous property setter (line 38). https://github.com/dotnet/aspnetcore/blob/905557eaca32f5ce9fd17a25e5187ceb7a75cf1c/src/Components/Web/src/Forms/InputText.cs#L32-L41

[mkArtakMSFT]

Thanks for raising this issue, @hakenr. There is already some work in progress for addressing issues from the bind-after syntax, and this may be another one in that bucket to be addressed in 7.0. We'll see.

[javiercn]

I am not 100% sure we can fit this into .NET 7.0. While I see the point being made, I think this was already possible without bind get, set, after. The main issue here was that the syntax was constrained to synchronous setters based on the helper methods our compiler used, but that was never a problem for something like InputBase which could have chosen to avoid using CreateBinder and replicated the code on the component. After all, it is already subscribing to the event in code, not with Razor syntax.

The feature that we built only makes it possible to do with Razor syntax, but the capability to do so in C# is already there since @bind is just syntactic sugar.

Nothing prevents us from moving the code inside CurrentValue and CurrentValueAsString into a handler method that we use instead of builder.AddAttribute(4, "onchange", EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString));

The only constraint would be that we would need to keep the order in which things happens the same, but it is perfectly doable.

If we choose to do this, it might be breaking for people who for some reason write to CurentValue or CurrentValueAsString directly, although I would be hard pressed to find a case outside of registering an event handler ourselves.

I would not design a new set of Input* components for this, it is not justified to split the world for this given the value we would get in return.

Given that this would be breaking I do not think we should do anything for .NET 7.0. I would be ok if other folks also are, with making a breaking behavior change in this area in .NET 8.0 to better support this scenario.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[ghost]

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.

[hakenr]

I'm proposing a PR for your consideration and approval:

I plan to introduce a new protected async Task SetCurrentValueAsStringAsync() method and relocate the entire logic from the set_CurrentValueAsString property setter to this method. This will include awaiting ValueChanged.InvokeAsync(Value); within the method.

protected async Task SetCurrentValueAsStringAsync(TValue value)
{
    var hasChanged = !EqualityComparer<TValue>.Default.Equals(value, Value);
    if (hasChanged)
    {
        _parsingFailed = false;
        Value = value;
        await ValueChanged.InvokeAsync(Value);
        EditContext?.NotifyFieldChanged(FieldIdentifier);
    }
}

The set_CurrentValueAsString property will now asynchronously trigger the new method without waiting for it to complete:

protected TValue? CurrentValue
{
    get => Value;
    set => _ = SetCurrentValueAsStringAsync(value);
}

We'll update all existing bindings to CurrentValueAsString (for instance, in InputBase) to utilize SetCurrentValueAsStringAsync() instead of the synchronous set_CurrentValueAsString. For example:

builder.AddAttribute(11, "onchange", EventCallback.Factory.CreateBinder<string>(this, async (string ___value) => await SetCurrentValueAsStringAsync(___value), CurrentValueAsString));

This approach ensures proper asynchronous handling of ValueChanged events while maintaining backward compatibility for all existing (third-party) components that rely on the CurrentValueAsString property for binding. It's a fallback to synchronous execution when needed.

Would this approach be acceptable to you? Are you interested in such a PR? I'll include comprehensive E2E tests and refine the code further.

@SteveSandersonMS? @javiercn?

[hakenr]

I'm proposing a PR that addresses the issue as described above https://github.com/dotnet/aspnetcore/issues/44105#issuecomment-1953227329