Skip to content

Popover: add Legacy mode (uses old MudPopoverService)#7497

Merged
ScarletKuro merged 3 commits intoMudBlazor:devfrom
ScarletKuro:popover_legacy
Sep 14, 2023
Merged

Popover: add Legacy mode (uses old MudPopoverService)#7497
ScarletKuro merged 3 commits intoMudBlazor:devfrom
ScarletKuro:popover_legacy

Conversation

@ScarletKuro
Copy link
Member

@ScarletKuro ScarletKuro commented Sep 11, 2023

Description

See for context #7434
TL;DR - A customer reported an issue with Blazor Server Side performance with the popovers (only the initial load), and we identified that PR #6953 was the cause. I intend to investigate and address the root cause when I have more time. Meanwhile, to provide a quick solution, we decided to introduce a backward compatibility option that allows users to easily switch to the old popover service (that contains it's own issues tho).

To enable the old popover:

services.AddMudServices(config =>
{
	config.PopoverOptions.Mode = PopoverMode.Legacy;
});

How Has This Been Tested?

Manually, #7434 (reply in thread)

Types of changes

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

Checklist:

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

@github-actions github-actions bot added the enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library label Sep 11, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 43.75% and project coverage change: -0.04% ⚠️

Comparison is base (91ebd58) 90.61% compared to head (981bc66) 90.57%.
Report is 6 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #7497      +/-   ##
==========================================
- Coverage   90.61%   90.57%   -0.04%     
==========================================
  Files         427      427              
  Lines       15198    15210      +12     
==========================================
+ Hits        13772    13777       +5     
- Misses       1426     1433       +7     
Files Changed Coverage Δ
...c/MudBlazor/Components/Popover/MudPopover.razor.cs 76.00% <ø> (ø)
src/MudBlazor/Services/Popover/PopoverOptions.cs 100.00% <ø> (ø)
src/MudBlazor/Components/Popover/MudPopoverBase.cs 65.38% <40.00%> (-34.62%) ⬇️
...rc/MudBlazor/Services/Popover/MudPopoverHandler.cs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ScarletKuro ScarletKuro marked this pull request as ready for review September 13, 2023 14:34
@ScarletKuro ScarletKuro requested a review from henon September 13, 2023 14:35
@henon
Copy link
Contributor

henon commented Sep 14, 2023

Interesting, I'd also love to know what it is that causes that initial delay. Any hunches? In any case, we'll have to be able to reproduce the problem, otherwise it would certainly be hard to test if the issue is gone after making changes or we'd fully rely on the OP of the bug report.

@ScarletKuro
Copy link
Member Author

ScarletKuro commented Sep 14, 2023

Any hunches?

I only see one difference currently:
Both methods on initialize create PopoverHolder and set the initial values, both add it to the dictionary, but the the old service is invoking native eventhandler which triggers a non awaitable InvokeAsync(StateHasChanged) while the new service invoking an observable with await InvokeAsync(StateHasChanged).
So my ideas is to try:
A) ask OP to remove the await and try
B) use ConfigureAwait(false) on this method, after this PR was merged (#7441) we do not call any JS on create therefore we don't need to run it on UI synchronization context.

@ScarletKuro
Copy link
Member Author

In any case, we'll have to be able to reproduce the problem, otherwise it would certainly be hard to test if the issue is gone after making changes or we'd fully rely on the OP of the bug report

Yeah, that's a big problem. OP says it happens only in their AWS cloud, but i wonder if that happens only on their specific codebase, because I'm sure other BSS folks would report this long time ago.

@ScarletKuro ScarletKuro merged commit ca6c54d into MudBlazor:dev Sep 14, 2023
@ScarletKuro ScarletKuro deleted the popover_legacy branch September 14, 2023 10:47
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants