[Enhancement] Support for async subscribers in the EventAggregator

[james1301]

Summary

Hi, as previously requested here: #832. Could this be reconsidered for implementation? Now that there is a AsyncDelegateCommand, it seems this would also be similarly useful. This increases test ability, without having to make subscribing methods public and creating async voids anonymous methods as a wrapper. It can still work as fire and forget when actually called.

API Changes

PubSubEvent.Subcribe(Func action);
PubSubEvent.Unsubcribe(Func action);

Intended Use Case

For unit tests predominantly so that the subscribe method can be extracted through mocking the PubSubEvent, and subsequently called without making the method public.

Also reduces need for async voids and extra overhead of that.

[brianlagunas]

This will be a hard sell. The purpose of the event aggregator is to send a fire and forget message to any number of listeners in the form of events. It really goes against the design of the event aggregator.

If your only reason for wanting this is to make testing the subscribed methods easier to test, just make them internal and expose your internals to your test libraries

[dansiegel]

Perhaps you could show a proposed snipped of code of what this would look like and help us understand what problem it is that you're trying to solve. Though I will be honest I'm very much in agreement with @brianlagunas here that the EventAggregator is supposed to be fire and forget. The publisher shouldn't care about who may or may not be listening.

[james1301]

Thanks guys, I agree with you that it should be fire and forget, but that doesn't stop it from having the mechanism to get hold of the the async task method does it? I try to avoid the internals visible thing, and it's a bit messy with strong naming. In this day and age, it must be quite common that somebody would require an async method from an event, and hopefully also nowadays there are people would want to test that scenario. The tests will often work as well, unless they are specifically putting something on a different thread that isn't mocked. The problem comes if that test ever fails, or you are testing an exception raised from that event, then it will pass the test, but crash the test runner as seems to be the case.

The anonymous wrapper for these subscriptions is also not very clean, it's adding extra overhead and makes unsubscribing more messy.

We have code where you do an anonymous async void method to get it so you don't end up with an async void method, which you are struggling to test properly anyway. Then you need to use the subscription token because you have an anonymous method you need to unsubscribe from.

` private SubscriptionToken resultToken;

public override void Destroy()
{
    base.Destroy();
    this.eventAggregator.GetEvent<RestartEvent>().Unsubscribe(this.Start);
    this.eventAggregator.GetEvent<ResultEvent>().Unsubscribe(this.resultToken );
}	

public override async Task OnInitialNavigatedTo(NavigationContext navigationContext)
{
    await base.OnInitialNavigatedTo(navigationContext).ConfigureAwait(true);
	
    this.eventAggregator.GetEvent<RestartEvent>().Subscribe(this.Start);
    this.resultToken = this.eventAggregator.GetEvent<ResultEvent>().Subscribe(async (x) => await this.ResultReceived(x).ConfigureAwait(true), ThreadOption.UIThread);
}

private void Start()
{
}

private async Task ResultReceived()
{
	return Task.CompletedTask;
}`

So as a workaround for this, I would probably just make it public, as it is, in effect public from the event aggregator call, a bit cleaner than the internals approach, but it would be cleaner being private. The code coverage still is going to complain about that async anonymous method, which you still can't test.

Would doing a discard, not keep an async task version still as fire and forget? I have toyed around with CommunityToolkit's Messenger, it does allow some async request/response type messages but still doesn't really allow these kind of situations. Plus it seems a little overly complicated and I guess not for quite the same purpose. This is one reason why I often avoid where possible to use events, an initial async subscription capability would be great.

[dansiegel]

I'm not sure what API you're referring to that should be made public. There is no async usage at all in the EventAggregator.

[james1301]

No I just meant as a workaround, rather than internal and internal visible to, I would just make the method Public at the moment so it can be called by tests. But that still leaves some uncovered testing code in the subscribing.

[dansiegel]

@james1301 I'm just not seeing anything from what you've provided that helps to sell me on this idea. If you can provide a spec of the API changes and show how that would be used so we can better understand the problem you're trying to solve that would be helpful.