MudDialog: Fix Dialog is null in IDialogReference

[ScarletKuro]

Description

Resolves #1839

Check my comment for more details https://github.com/MudBlazor/MudBlazor/issues/1839#issuecomment-1211460905

I guess this is the best way to solve it without making drastic / breaking changes to dialogs system.

The RenderCompleteTaskCompletionSource was made internal on purpose so it wouldn't be visible to the end developer. Had no other choice but shove it inside IDialogReference.

I didn't mark Show with Obsolete, because current project settings wouldn't allow to compile when an obsolete method is used. It would require changing everything - tests and docs. Also some public methods that use Show are also not awaitable, for example MudMessageBox.Show is awaitable, and can be changed to ShowAsync, meanwhile MudDialog.Show is not awaitable, so I didn't want to risk changing too much.

How Has This Been Tested?

Unit test and by checking DialogPassingDataExample

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• [x] The PR is submitted to the correct branch (dev).
• [x] My code follows the code style of this project.
• [x] I've added relevant tests.

[codecov[bot]]

Codecov Report

Base: 90.14% // Head: 91.49% // Increases project coverage by +1.35% :tada:

Coverage data is based on head (e525d98) compared to base (b559b3d). Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #5101      +/-   ##
==========================================
+ Coverage   90.14%   91.49%   +1.35%     
==========================================
  Files         372      381       +9     
  Lines       13380    14944    +1564     
==========================================
+ Hits        12061    13673    +1612     
+ Misses       1319     1271      -48     

Impacted Files	Coverage Δ
src/MudBlazor/Components/Dialog/DialogReference.cs	77.77% <ø> (ø)
...lazor/Components/MessageBox/MudMessageBox.razor.cs	25.00% <0.00%> (ø)
src/MudBlazor/Services/DialogService.cs	86.86% <70.58%> (-3.50%)	:arrow_down:
...lazor/Components/Dialog/MudDialogProvider.razor.cs	95.00% <100.00%> (+0.55%)	:arrow_up:
src/MudBlazor/Services/IIJSRuntimeExtentions.cs	70.58% <0.00%> (-29.42%)	:arrow_down:
src/MudBlazor/Extensions/TaskExtensions.cs	28.57% <0.00%> (-26.99%)	:arrow_down:
src/MudBlazor/Themes/Models/Palette.cs	78.08% <0.00%> (-6.08%)	:arrow_down:
...MudBlazor/Components/Collapse/MudCollapse.razor.cs	93.10% <0.00%> (-4.04%)	:arrow_down:
src/MudBlazor/Services/Scroll/ScrollManager.cs	40.00% <0.00%> (-2.86%)	:arrow_down:
src/MudBlazor/Components/Highlighter/Splitter.cs	90.47% <0.00%> (-2.39%)	:arrow_down:
... and 78 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[just-the-benno]

Before we look into that. I'd like to wait for #4895 to be merged

[ScarletKuro]

The #4895 is merged. This can be merged and then for .net 7 as breaking change we can remove the non async api and the component will work as intended.

[mikes-gh]

@henon What do you think. Can we merge this? Do we need more coverage?

[henon]

Of course if every of the new methods could be covered by a test that would be best. Otherwise we wouldn't notice if they stopped working at some point in the future due to other changes.

[ScarletKuro]

The problem is, that the unit test behaves differently compared to real world in some scenario. In unit test the old non async methods will have Dialog not empty, while in real application without any delays and awaiting of Dialog the property will be null. So what exactly do we test then? I can just copy paste the tests of the non async method one, but that doesn't really test anything new, the method is almost the same as the old one with the exception that we await now when the render is completely done on which there is one unit test already. Any ideas? Maybe someone has an idea how to better test this.

[henon]

Right, we can't test things that can't be tested with bUnit. But if someone would remove any of these methods it would potentially break user code without us noticing. So just by calling them we already provide more security if nothing else can be tested.

[ScarletKuro]

Ok, will do then

[ScarletKuro]

Added tests (it's duplication of the sync version with using ShowAsync). I also used little different approach for IDialogReference for async Instead of

IDialogReference dialogReference = null;
await comp.InvokeAsync(async () => await dialogReference = service?.ShowAsync<DialogOkCancel>());

I used this

var dialogReferenceLazy = new Lazy<Task<IDialogReference>>(() => service?.ShowAsync<DialogOkCancel>());
await comp.InvokeAsync(() => dialogReferenceLazy.Value);
var dialogReference = await dialogReferenceLazy.Value;

[henon]

Awesome, very well done. Thanks!