jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
11.14k stars 1.18k forks source link

singleton NotificationHandler #810

Closed kbilsted closed 1 year ago

kbilsted commented 1 year ago

Hi I'm toying around with a rich console application. I've started using MediatR to loosen coupling.

How do I best implement the mainwindow (the view) as a NotificationHandler (it get notified with data and status updates). I've read in other issues here on GH, that you say that notification handlers should be stateless. But that means they need to get the gui injected into them - which feels a bit tedious compared to simply implementing them. Also it forceds the gui to have public methods for something that could be private.

And when I make a specific class for eventhandling, the singleton MainWindow gets created as a new instance

    public class MainWindowEventHandlers :INotificationHandler<ReWasCreated> {
        private readonly MainWindow mainWindow;

        public MainWindowEventHandlers(MainWindow mainWindow) {
            this.mainWindow = mainWindow;
        }

        public Task Handle(ReWasCreated notification, CancellationToken cancellationToken) {
            mainWindow.workList.UpdateItems(notification.Res);
            return Task.CompletedTask;
        }
    }

I would have thought it be wrong to use the Activator to create instances, since it does not respect the IOC framework at hand. See https://github.com/jbogard/MediatR/blob/master/src/MediatR/Mediator.cs#L38

Im using in my main

var host = new HostBuilder()
        .ConfigureLogging((ctx, lb) => {
            lb.AddConfiguration(config);
            lb.AddFileLogger();
        })
        .ConfigureServices((hostContext, services) => {
            services.AddMediatR(Assembly.GetExecutingAssembly());

            services.AddSingleton<MainWindow, MainWindow>();
            ...

many thanks

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

asimmon commented 1 year ago

I'm very late to this thread @kbilsted, but in my experience, notification handlers and request handlers should be stateless and consume stateful dependencies. I always register my handlers as transient to ensure they don't preserve any state, but that's my preference.

You're going to introduce a strong coupling between your business logic and UI layer, forcing you to expose public methods on your main window. But that's not the fault of MediatR. Your design made you do this.

The way I see it, the stateless notification handler should publish the changes to a stateful singleton object, and the UI layer could listen to this object using various patterns or techniques: event handlers, BlockingCollection<T>, asynchronous Channel<T>, IAsyncEnumerable<T>... there are plenty of choices here.

In the diagram below, the MainWindow don't have to expose anything and is not aware of any MediatR handler. They can live in separate assemblies for better testability:

image

@jbogard sorry for reopening this stale issue, feel free to close it if you want to.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stalled for 14 days with no activity.