MudForm and others: rename async methods #8875
Conversation
henon
added
API change
|
Should we rename Validate -> ValidateAsync? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #8875 +/- ##
==========================================
+ Coverage 89.82% 90.14% +0.31%
==========================================
Files 412 421 +9
Lines 11878 12288 +410
Branches 2364 2428 +64
==========================================
+ Hits 10670 11077 +407
+ Misses 681 662 -19
- Partials 527 549 +22 ☔ View full report in Codecov by Sentry. |
|
Maybe we should also make EvaluateForm and Update async and rename |
…ng where possible
|
@ScarletKuro I made every method async and eliminated async Task discarding where possible except IForm.Update. Here is my reasoning embedded in a code comment in the method itself: /// <summary>
/// Called by any input of the form to signal that its value changed.
/// </summary>
/// <param name="formControl"></param>
void IForm.Update(IFormComponent formControl)
{
// Note: We don't want to make IForm.Update async because it makes no sense for the inputs to wait
// for form evaluation, especially when multiple input parameters change which results in several form
// updates which are debounced anyway via a timer.
// Validation exceptions are observed via AndForget() which reports exceptions via MudGlobal.UnhandledExceptionHandler
EvaluateFormAsync().AndForget();
} |
henon
changed the title
| @@ -15,7 +16,7 @@ public bool IsValid | |||
| { | |||
| get | |||
| { | |||
| Validate(); | |||
| ValidateAsync().Wait(); | |||
There was a problem hiding this comment.
Won't this deadlock on WASM considering that .GetAwaiter().GetResult() does deadlock on WASM? And under the hood it's using Wait and this method in general causing thread pool starvation?
There was a problem hiding this comment.
I don't know, how would you synchronously await the validation result on WASM?
There was a problem hiding this comment.
If it is not possible we'll have to keep this PR for v8
There was a problem hiding this comment.
I think the user of the validator should directly call ValidateAsync instead of using IsValid. ValidateAsync should return a Task<bool>
There was a problem hiding this comment.
I don't know, how would you synchronously await the validation result on WASM?
That's the most annoying part :) you can't do it reliable on WASM without await like Wait, .GetAwaiter().GetResult(), .Result etc because this is all blocking calls to wait for the task to finish and this is not supported: dotnet/aspnetcore#18092 (comment)
| @@ -10,7 +11,7 @@ public bool IsValid | |||
| { | |||
| get | |||
| { | |||
| Validate(); | |||
| ValidateAsync().Wait(); | |||
There was a problem hiding this comment.
Same comment as in DataGridRowValidator
I guess it makes sense. public IForm Validator { get; set; } = new DataGridRowValidator();yet the interface has internal properties methods that are crucial. |
For example, it feels like the IsValid should either be a method like in datacontract OR it should have no logic, but like in fluentvalidation that some method call is calculating the |
|
This is a mega-project. Scheduling it for v8 |
| public bool IsTouched | ||
| { | ||
| get => _touched; | ||
| set {/* readonly parameter! */ } |
There was a problem hiding this comment.
Can the setter be removed entirely, or is it needed for something like ParameterState?
There was a problem hiding this comment.
I guess it was coded like this to make sure everyone gets it that it is intentionally readonly. But I guess there are more modern ways to do it, I just don't know them.
| } | ||
|
|
||
| private void EvaluateForm(bool debounce = true) | ||
| private Task EvaluateFormAsync(bool debounce = true) | ||
| { | ||
| _timer?.Dispose(); | ||
| if (debounce && ValidationDelay > 0) | ||
| _timer = new Timer(OnTimerComplete, null, ValidationDelay, Timeout.Infinite); |
There was a problem hiding this comment.
If you need a timer which plays well with async code, consider the PeriodicTimer.
| { | ||
| await Task.WhenAll(_formControls.Select(x => x.Validate())); | ||
| await Task.WhenAll(_formControls.Select(x => x.ValidateAsync())); |
There was a problem hiding this comment.
These two Task.WhenAll's could be combined if you add the tasks to a List<Task> then WhenAll on that list.
|
|
||
| EvaluateForm(debounce: false); | ||
| await Task.WhenAll(_formControls.Select(x => x.ResetValidationAsync())); | ||
| await Task.WhenAll(ChildForms.Select(x => x.ResetValidationAsync())); |
There was a problem hiding this comment.
These two Task.WhenAll's could be combined into a List<Task> with a single WhenAll
| ParentMudForm.EvaluateForm(); // Need this to refresh the form state | ||
| } | ||
| ParentMudForm?.ChildForms.Remove(this); | ||
| ParentMudForm?.EvaluateFormAsync().AndForget(); // Need this to refresh the form state. AndForget reports errors via MudGlobal.UnhandledExceptionHandler |
There was a problem hiding this comment.
Should this be changed to DisposeAsync and IAsyncDisposable? I believe the _timer also has a DisposeAsync()
|
Thanks for the review @jperson2000, lots of good comments. This PR is on hold for now though. |
Description
I originally wanted to rename MudForm's IsValid and IsTouched but then decided to leave them be for now becahse
EditFormandFluentValidationboth useIsValidand if we changeMudFormtoValidit would actually increase inconsistency instead of decreasing it. I also decided to leave IsTouched alone. We might want to change it again after reworking form in v8 so it doesn't make much sense to touch it now.But
IFormComponent.IsForNullcaught my eye, I believeForIsNullis a better name so I renamed it.Also overhauld MudForm's asynchronous validation and renamed several methods to bear the Async postfix.
Changes
IsForNulltoForIsNullValidatetoValidateAsyncValidatetoValidateAsyncValidatewithValidateAsyncand awaitResetValidationwithResetValidationAsyncand awaitType of Changes
Checklist
dev).