nestjs / cqrs

A lightweight CQRS module for Nest framework (node.js) :balloon:
https://nestjs.com
MIT License
848 stars 150 forks source link

EventBus and @EventHandler decorator #98

Closed JulienKyu closed 5 years ago

JulienKyu commented 5 years ago

I'm submitting a...


[ ] Regression 
[X ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I have two event handler class with the same name for the same event but with different logic and in different module. When my bus receive the event concerned, only last event handler registered handle these. If I change the class name of one of EventHandler it's works.

For information, Explorer Service (events) has no problem to get these event handlers.

Expected behavior

If I add a @EventsHandler decorator, the class handle my event whatever his name.

Minimal reproduction of the problem with instructions

Two event handlers with the same name for the same event but in different module.

What is the motivation / use case for changing the behavior?

I have multiple bounded context (one per module) and it's more easy for me if the event handler class is the name of event with EventHandler suffix.

If I add bounded context name on prefix, its works, but the name of class is sooooooo long.

Environment


Nest version: 6.1.1
Nest CQRS version: 6.0.0


For Tooling issues:
- Node version: v10.6.0  
- Platform:  Mac 


kamilmysliwiec commented 5 years ago

Could you please provide a minimal repository which reproduces your issue?

JulienKyu commented 5 years ago

Could you please provide a minimal repository which reproduces your issue?

Yes I can, I wait just weekend to do this. Thanks

FERNman commented 4 years ago

Hi @kamilmysliwiec ,

I'm currently experiencing the same problem as @JulienKyu. What he meant by his issue is that when two modules have an event handler with the same name, the DI token for both is the same. Therefore only the one registered first will be called, but twice.

I suppose this is something more people are experiencing because it is common to name event handlers by the events they are handling.

Here is a reproduction: https://stackblitz.com/edit/nest-duplicate-event-handlers-reproduction

Although I'm not that deep into the NestJS DI system, I guess it should be possible to scope the event handlers in the ExplorerService to their module.

For everyone else stumbling across this: I'm currently using the same workaround as per the original issue, just prefixing all event handlers with the module name.

JulienKyu commented 4 years ago

@FERNman

To fix temporary this issue I use a custom class named "Projection" with @EventsHandler(...) and this method.

public getEventHandler<E extends IEvent>(event: E): IEventHandler<E> {
    const eventHandlerName = event.type + 'Handler';
    try {
        return this.moduleRef.get(eventHandlerName);
    } catch (e) {
        this.logger.error(eventHandlerName + ' not implemented');
    }
}

I have also register all EventHandler named by the event name followed EventHandler suffix.

With this strategy, I can use each event handler as same name for same event splitted by module.

Dont forget to add type property to all events like this:

....
public type: string;

public constructor() {
    this.type = this.constructor.name;
}
....
JulienKyu commented 4 years ago

@kamilmysliwiec

Could you please provide a minimal repository which reproduces your issue?

Here is a reproduction: https://github.com/FERNman/nest-duplicate-event-handler

Could you please re open this issue ? or you want I create another issue ?

Thanks a lot

imxm commented 3 years ago

is it fixed, because I am having the same issue?

JulienKyu commented 3 years ago

Hi,

I dont think it's fixed,

examples of simple interest to illustrate the motivation:

Domain (Module) -Pricing --OrderCreatedEventHandler ---Update some price -ProductCatalog --OrderCreatedEventHandler ---Update inventory

For now, I have not found better method than this to make this example work, with official documentation code sample, OrderCreatedEvent has handle 2 times by first handler named OrderCreatedEventHandler in the module Domain.

@kamilmysliwiec please help us

Thanks a lot

kobyzskyi commented 3 years ago

Any updates with it? I have the same issue. Version of CQRS package - 8.0.0

IRCraziestTaxi commented 3 years ago

I just ran into (I think) the same problem with this. I have two different queries (classes that are the argument for two different @QueryHandlers) in two different modules, but the incorrect query handler was being used for the query I was calling via the query bus. I double checked that all my imports were correct; that is, the query/handler from the incorrect module was not being imported, but rather the one local to the module. Changing the name of one of the query classes fixed the issue. It does seem like a bug to me.