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

WIP: #247 Down the rabbit hole replacing the resolution with injection #252

Closed
wants to merge 5 commits into from

Conversation

dadhi
Copy link
Contributor

@dadhi dadhi commented Apr 6, 2018

  • Tests are green.
  • Samples are running fine! Though the constrained stuff is turned off for Ninject and Unity.

added: Added both typed and un-typed IRequestMediator, INotificationMediator to represent the capabilities and the entry points of MediatR

changed: Request / Notification wrappers to RequestMediator / NotificationMediator; Handle methods changed to Send / Publish.
changed: Mediator.PublishCore method was renamed to PublishBehavior to be more friendly, parameters in notification mediator were renamed to.
changed: DryIocZero has a few resolution roots, for request and notification mediators only. That's how Pure DI can look like :)

removed: IMediator and ServiceFactory registrations from Tests and Samples, cause not needed
removed: Caches from Mediator, relying on IoCs lifetime management and cache

…njection where possible

Tests are green.
Samples are running fine! Though it was needed to turn off constrained stuff for Ninject and Unity.

changed: Request / Notification wrappers to RequestMediator / NotificationMediator to represent the capabilities; Handle methods changed to Send / Publish.
added: Added both TRequest typed and un-typed IRequestMediator, INotificationMediator for consistency
changed: Mediator.PublishCore method was renamed to PublishBehavior to be more friendly, parameters in notification mediator were renamed to.
changed: DryIocZero has a few resolution roots, for request and notification mediators only. That's how Pure DI can look like :)

removed: IMediator and ServiceFactory registrations from Tests and Samples, cause not needed
removed: Caches from Mediator, relying on IoCs lifetime management and cache
@dadhi
Copy link
Contributor Author

dadhi commented Apr 6, 2018

What is still missing or can be added:

  • Doc comments on request and notification mediators

  • May be extend IMediator interface with typed Send(TRequest ...) and un-typed Publish(INotification ..). It will allow more performant (no MakeGenericType) variant for sending request, though at cast of specifying type arguments when calling Send Sigh :-( for no constraint inference in C# yet.

  • Sample with Pure DI (without IoC lib). Much easy to add it now

…eptions

removed: Unnecessary IMediator resolution root in DryIocZero sample, now it is clean and shiny
reverted: Unnecessary changes in tests
@dadhi
Copy link
Contributor Author

dadhi commented Apr 7, 2018

After this PR, the IMediator interface becomes just a glue / sugar between the mediator capabilities and IoC lib in the resolution root. Basically, it is a Service Locator of capabilities, with all pros and cons of indirect service location.

More often you may want to inject IRequestMediator<TRequest, TResponse> or INotificationMediator<TNotification> directly into your service, controller, etc. It is much more SOLID and testable, and works without IoC container. If User don't need notifications, no need to pay for it.

Given that Mediator is service locator, it can be rewritten to express this clearly:

interface IMediator
{
    IRequestMediator<TReq, TResp> GetRequestMediator<TReq, TResp>();

    INotificationMediator<TNotification> GetNotificationMediator<TNotification>();
}

The default Mediator implementation will work like Today by wrapping IoC ServiceFactory delegate.

Then we may add Send and Publish methods into MediatorExtensions class.

PublishCore method is default notification strategy, and therefore may be moved to NotificationMediator (default) implementation.

# Conflicts:
#	src/MediatR/INotificationHandler.cs
#	test/MediatR.Tests/SendVoidInterfaceTests.cs
@jbogard
Copy link
Owner

jbogard commented Apr 10, 2018

I'm not sure I'd inject the individual mediators unless I was only sending one command/notification. Part of the point was instead of IFoo<T> you had IFoo.DoSomething<T>. It's generally more annoying to deal with the generic types vs. generic methods.

Not discounting the value of the composition roots - it's just generally more annoying from the outermost layer.

@dadhi
Copy link
Contributor Author

dadhi commented Apr 10, 2018

You may do both, inject IMediator as before, or be more specific, it is up to user.
You may also control the pipeline lifetime.
Injection vs service location simplifies DI.

added: missing xml docs;
moved: PublishCore to NotificationMediator;
cleanup
@dadhi
Copy link
Contributor Author

dadhi commented Apr 18, 2018

@jbogard,

I've likely done with this PR. Confirmed for myself couple of things about IoC lib integration and my DryIoc family.
Feel free to accept or dismiss.

var mediator = container.Resolve<IMediator>();

return mediator;
return new Mediator(container.Resolve);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd still want to register IMediator, no?

@dadhi
Copy link
Contributor Author

dadhi commented Apr 18, 2018

For the sake of this sample and MediatR itself, no.

If you want to inject IMediator on consumer side, then register it as any other service.

@jbogard
Copy link
Owner

jbogard commented Apr 18, 2018

So the purpose of the samples is to show how, in normal usage, you'd use MediatR. Consumers would still want to depend on IMediator not the sub-mediators, because as those interfaces are generic, you'd have to have a dependency-per-thing you were handling. One of the main design goals is to not do that, otherwise I'd have just depended directly on IHandler<,> so long ago.

@dadhi
Copy link
Contributor Author

dadhi commented Apr 18, 2018

This is fine with me.

May be just an IHandler is not enough at the moment, given the pipeline behaviors, etc.

My PR is just to fill the gaps in abstractions from IHandler to the IMediator.

If you will decide to merge, I can revert back samples to register IMediator and ServiceFactory.

Copy link
Contributor

@remcoros remcoros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this also gets rid of the type cache dictionary in mediator.

Maybe not register the individual Req/Not.Handler, but move them to 'Internal' namespace (and leave the types public).

protected virtual Task PublishCore(IEnumerable<Task> allHandlers)
{
return Task.WhenAll(allHandlers);
var mediator = (INotificationMediator<TNotification>)_serviceFactory(typeof(INotificationMediator<TNotification>));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be 'notification.GetType() / MakeGenericType' like the original code and how Send does it. So you don't have to pass concrete types (e.g. if you pass an 'IEvent' it resolves the actual type of the object, not of IEvent).

@dadhi
Copy link
Contributor Author

dadhi commented Apr 18, 2018

@remcoros,

I like how this also gets rid of the type cache dictionary in mediator.

Yes, you are now in full control of lifetime and scoping of mediator(s). No assumptions. Just use your lovely IoC registration features.

This should be 'notification.GetType() / MakeGenericType' like the original code and how Send does it.

May be you're right.

I was thinking to get rid off reflection as much as possible. The whole point of explicit TRequest, TResponse, and TNotification type parameters is to provide the statically-typed routing to the matching handler. The client code, just by looking at Send and Publish method signatures should see the requirements for inputs. Otherwise, it is leaky abstraction - I should assume that mediator uses GetType on input object.

I would've do the same thing for Send, but due the C# limitations we are losing inference, so be it.

@jbogard
Copy link
Owner

jbogard commented Apr 18, 2018

So I think at the end, this would still increase the registrations people would use? Or they'd have two entry points, one a non-generic interface (IMediator) or if they wanted a specific mediator for a specific request/response type they'd want that?

At that point, I'm not sure the generic interfaces are even mediators any more. The mediator pattern is to encapsulate communication, and now what you're communicating is in the type as opposed to the method. This lets for example a web API controller only depend on IMediator.

Currently, you don't have many registrations required for the "normal" usage. You'd do IMediator, the ServiceFactory, and handlers.

@dadhi
Copy link
Contributor Author

dadhi commented Apr 18, 2018

@jbogard,

It would be plus 1 or plus 2 registrations per whole setup. And you need to understand why are you doing those registrations, which is good in my opinion.

But look at registrations in the Examples. It is kinda hard to compare across the containers, even blindly by code size. So adding only two things is good or bad? I am Ok to have those in DryIoc and DIZero.

My view, I would rather have two additional registrations with clear goal and control, than a internal magic glue. But it is my view only.

changed: Mediator default impl. uses Activator.CreateInstance as before PR - no need for additional registrations
added: MediatorExtensions with Send and Publish to IMediator
added: the DryIoc(Zero) and NoIoC examples use custom InjectingMediator with registered IRequestMediator and INotfiicationMediator - so both approaches valid
removed: IRequestMediator and INotfiicationMediator from the rest of Examples and Tests
reverted: Rest of examples are registering IMediator and ServiceFactory as prio PR
@dadhi
Copy link
Contributor Author

dadhi commented Apr 23, 2018

@jbogard,

Hi again :)

I have reverted default registration back (almost), so no need for IRequestMediator and INotificationMediator registrations.
But you can opt-in either by directly injecting them, or via simple IMediator implementation as in
example.

The option is more simple now because of role split between IMediator (infrastructure creation glue) and MediatorExtensions (Publish and Send sugar).

@dadhi
Copy link
Contributor Author

dadhi commented Jun 5, 2018

@jbogard, I will close the PR now as 5.0 is out.

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

Successfully merging this pull request may close these issues.

3 participants