[Blazor] InputBase - SetCurrentValueAsync + SetCurrentValueAsStringAsync (fix for #44105)

[hakenr]

Proper Asynchronous Handling for ValueChanged Callback

Fixes #44105

Description

• Failing E2E tests were introduced in the first commit (the tests demonstrate just one of the issues related to the original implementation).
• The fix was introduced in the second commit (tests passed).
• Backwards compatibility is preserved.
• New public APIs have been introduced:

  Microsoft.AspNetCore.Components.Forms.InputBase<TValue>.SetCurrentValueAsStringAsync(string? value) -> System.Threading.Tasks.Task!
  Microsoft.AspNetCore.Components.Forms.InputBase<TValue>.SetCurrentValueAsync(TValue? value) -> System.Threading.Tasks.Task!
  

I'm willing to continue collaborating on this PR if I receive a code review and further refinements are needed.

cc @jirikanda

[dotnet-policy-service]

Thanks for your PR, @hakenr. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[hakenr]

The third commit fixes InputBaseTest (and related) where the SetCurrentValueAsStringAsync() method was already used.
It seems these tests can be refactored to use the new asynchronous InputBase methods.
Let me know if I should do so.

[hakenr]

Now it seems the failing tests are unrelated (HTTP/2, HTTP/3).

[hakenr]

/azp run

[azure-pipelines]

Commenter does not have sufficient privileges for PR 54279 in repo dotnet/aspnetcore

[dotnet-policy-service]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[MackinnonBuck]

Thanks for the contribution, @hakenr. We're interested in taking this change, but it has to go through API review first. Would you be willing to fill out a new issue using the API Proposal template? Its content would probably be quite similar to what you already wrote in #44105. This will allow us to prioritize reviewing the changes here.

[hakenr]

Thanks for the contribution, @hakenr. We're interested in taking this change, but it has to go through API review first. Would you be willing to fill out a new issue using the API Proposal template? Its content would probably be quite similar to what you already wrote in #44105. This will allow us to prioritize reviewing the changes here.

I will try to submit the API Proposal as requested.

[javiercn]

@hanenR sorry it took us a while to get back to this PR.

We had an "intense" discussion within the team a couple months back and we had to summarize the discussion.

We discuss the possibility of awaiting ValueChanged.InvokeAsync and we've come to the conclusion that we prefer the most scoped solution of not awaiting it and instead reporting the error through the renderer API.

The reason for this is that it is very easy to get into a situation where if your binding does async work, any high throughput input will result in the state between the control and the input getting out of sync.

For example, having an input that does some async work and then capitalizes the value would cause a sequence of events like
a -> async work(a)
ab -> async work(ab)
async work(ab) completes -> AB
async work(a) completes -> A

Right now there's no support for cancellation on InvokeAsync so we think it's best to recommend that bindings don't do async work as part of the binding and that any long running work is handled separately in a queue with cancellation.

We would still take this PR if we scope down the change to avoid the await ValueChanged.InvokeAsync(Value); and use _renderHandle.DispatchExceptionAsync to flow the exception correctly.

[dotnet-policy-service]

Hi @hakenr.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

[hakenr]

@javiercn, as I already said in #59477 (the API Proposal requested)

Thanks for taking the time to discuss this. I understand your concerns about changing the behavior of existing components and the extra work required to cover all scenarios.

Unfortunately, the proposed solution of handling and dispatching the exception still follows the fire-and-forget approach, without giving derived components the opportunity to handle it more effectively.

Did you consider an option where the default processing remains synchronous (fire-and-forget, as it is now) while introducing an asynchronous alternative (such as SetCurrentValueAsStringAsync()) to allow derived components to take responsibility and handle it asynchronously?

(Unfortunately, I don't align with the currently proposed solution of "handling and dispatching the exception only," as it doesn't fix the problem but merely patches one of its symptoms. I’m not willing to invest my effort in such a pull request or put my name behind it, sorry.)