Mediator icon indicating copy to clipboard operation
Mediator copied to clipboard

Update IRequestHandler to return ValueTask instead of ValueTask<Unit>

Open Dreamescaper opened this issue 2 years ago • 15 comments

I assume that this type was added for compatibility with MediatR. However, it is removed in latest MediatR versions - which is a good thing in my opinion, as it is much cleaner to write Task return type instead of Task<Unit>.

Does it make sense to remove it in the next major version?

Dreamescaper avatar Jul 06 '23 13:07 Dreamescaper

Probably, I'll look into it as part of 3.0. It makes sense to keep up with MediatR API as much as possible

martinothamar avatar Aug 22 '23 17:08 martinothamar

Any update on this? IT's one of the (small) obstacles that we are currently facing migrating from MediatR

zyofeng avatar Jul 30 '24 09:07 zyofeng

Any update on this? IT's one of the (small) obstacles that we are currently facing migrating from MediatR

I think if it's so important to migrate then u can use an adapter pattern

MaxLevs avatar Jul 30 '24 09:07 MaxLevs

Im currently using the following interface as a workaround, not ideal though

public interface IRequestHandler<in TRequest> : IRequestHandler<TRequest, Unit>
    where TRequest : IRequest<Unit>
{
    new ValueTask Handle(TRequest request, CancellationToken cancellationToken);
    
    async ValueTask<Unit> IRequestHandler<TRequest, Unit>.Handle(TRequest request, CancellationToken cancellationToken)
    {
        await Handle(request, cancellationToken);
        return Unit.Value;
    }
}

zyofeng avatar Jul 30 '24 10:07 zyofeng

This is also our last (small) obstacle to move over from mediatR with our projects. That being said is there anything we can do to move this along? Maybe someone can point me in the correct direction to develop/open a pr for this?

Nouwan avatar Apr 24 '25 21:04 Nouwan

I haven't looked into this much yet, but I'm guessing it's going to be hard to avoid dropping that Unit somewhere since the pipeline behaviors will still have to keep TResponse. Here is a rough outline of what needs to be done/investigated:

  • Update IRequestHandler interface
  • Update CompilationAnalyzer to find the new flavor of requesthandler (there are probably places where we expect 2 type parameters)
  • Source generate new ValueTask Send<TRequest>(TRequest request, ...); overloads
  • Add message handler wrappers that have no TResponse to match IRequestHandler (find a cheap way to drop Unit coming from pipeline)

Does anyone here want to work on this?

martinothamar avatar Apr 29 '25 05:04 martinothamar

I would like to try and implement this. There are 2 caveats to this however. I'll be going on vacation this Friday and will be offline for at least 2 weeks. I could start somewhere after that. I'm also unfamiliar with the project and haven't done much with source code generators before but it seems like a nice challenge.

If there is someone else who could pick this up sooner or has more experience I won't be demoralized/ impacted if you give it away at any point in the process :).

Nouwan avatar Apr 29 '25 11:04 Nouwan

I'm available to pick this up and work this out

mvput avatar Jul 09 '25 06:07 mvput

@martinothamar yesterday I was working on this feature and I want to know your take on the following. I think the most correct way to implement this is to completly ditch the TResponse for the source generation.

  • ISender will have a method Send which returns a ValueTask
  • IRequest and ICommand without response type, same for IRequestHandler and ICommandHandler

The pipeline behavior without a TResponse generic constraint

public interface IPipelineBehavior <TMessage> where TMessage: notnull, IMessage
{
    ValueTask Handle(TMessage message, MessageHandlerDelegate <TMessage> next, CancellationToken cancellationToken);
}

For example you have a validation pipeline you can either implement both or either one of them.

public class ValidationPipelineBehavior: IPipelineBehavior <TRequest> ,
    IPipelineBehavior <TRequest, TResponse>
  {
      ValueTask Handle(TMessage message, MessageHandlerDelegate <TMessage> next, CancellationToken cancellationToken);
      ValueTask<TResponse> Handle(TMessage message, MessageHandlerDelegate <TMessage, TResponse> next, CancellationToken cancellationToken);
  }

mvput avatar Jul 15 '25 05:07 mvput

Do you mean adding new overloads to ISender etc? I think that is what MediatR does, except that there is only 1 method in the pipeline behavior and then the "handler wrapper" for responseless messages/handlers just drops the unit. I'm fine with that approach I think

martinothamar avatar Aug 05 '25 06:08 martinothamar

@martinothamar correct MediatR wraps the handler en ditches the unit. The other option (which i slightly prefer) is overload ISender and also create a pipeline behavior without unit, so no need for wrapping the handler and dropping unit.

mvput avatar Aug 12 '25 19:08 mvput

How will we determine the order of behaviors?

options.PipelineBehaviors = [typeof(BehaviorWithResponse0), typeof(BehaviorWithoutResponse0), typeof(BehaviorWithResponse1)];

Or the DI equivalent. Unless I'm misremembering completely we just get a list of services from DI and the order is correct due to order of registration or config

martinothamar avatar Aug 13 '25 07:08 martinothamar

I think the order of either the behavior with response or without, so the order remains the same as currently.

mvput avatar Aug 13 '25 11:08 mvput

A small update, got everything working. Now testing and cleaning up the code. Hopefully ready for PR later this week.

mvput avatar Sep 22 '25 19:09 mvput

I've been also waiting for this as well! The package is already really useful! But, it would be even better! And I believe that it would attract more perfectionist developers like myself!

KarimovJavoxir avatar Sep 24 '25 10:09 KarimovJavoxir