Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not working as expected, cause too much render on the component which suscribe #4

Open
julienGrd opened this issue Oct 3, 2019 · 3 comments

Comments

@julienGrd
Copy link

Hi @mikoskinen and thanks for your work !

However i see a huge problem in your library.
Indeed i faced a performance issue with my app, and i see some of my components are definitively render too much (dotnet/aspnetcore#14684)
i debug this and i see each time one of the event aggregator (whatever the parameters) publish, all of the components wich suscribe (whatever the parameters) are re-loaded (i see them with put some trace on the ShouldRender when the value is true, when i traced back the call stach i arrive on the publish of components).

I precise i see this behavior is client-side (web assembly)

so if you have a component C1 wich publish the a message of type T1 and and component C2 which suscribe of message of type T2, C2 is reloaded when C1 publish, which make no sense, normally the type is used to isolate the different case.

In my case, the components was reloaded 8 time instead of 1. I don't take time to debug your code so i don't know what is causing this issue. However, i see you try to called automatically the StateAsChanged in your code, i think it's a bad idea, In my case sometime i don't want to relad my components (some are very huge) and can be a performance problem event without this bug. at least provide an option to disable it (or better, as a parameter of the publish).

Maybe i can make some PR if you need help.

Good luck

Julien

@mikoskinen
Copy link
Owner

Thanks Julien for the report. I wonder, are you able to provide a simple repro for the issue?

As you mentioned, the library shouldn't be calling StateHasChanged automatically anymore. It's a leftover from the early days of Blazor to help get around around some early bits.

@julienGrd
Copy link
Author

Thanks for your reactivity !

i push a simple repro here : https://github.com/julienGrd/BlazorWebAssemblyBugEventAggregator

  • launch the app and open javascript console
  • there is a ListenerComponent 1 which listen message1, send by the clic of SendMessage1
  • there is a ListenerComponent 2 which listen message2, send by the clic of SendMessage2
  • if you clic on send message 1, both Listener Component are re-render (there is some console.writeline you can see)
  • same thing with message 2

Hope it help, good luck !

@rborosak
Copy link

@mikoskinen The problem is in PublishAsync method of EventAggregator..
First you get all handlers

handlersToNotify = _handlers.ToArray();

Then you get and execute all the tasks from handlers that can handle published message

var tasks = handlersToNotify.Select(h => h.Handle(messageType, message));
await Task.WhenAll(tasks);

And the problem is in the next line.

foreach (var handler in handlersToNotify.Where(x => !x.IsDead))

In this foreach loop you are invoking StateHasChanged for every live handler so every component gets rerendered.

I think you should filter out the handler at the begining
handlersToNotify = _handlers.Where(x => x.Handles(messageType)).ToArray();

and then change from
var deadHandlers = handlersToNotify.Where(h => h.IsDead).ToList();
to
var deadHandlers = _handlers.Where(h => h.IsDead).ToList();
so you check if there are any dead handler no matter of message type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants