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

fix(event-bus): event id is optional #926

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

Sikora00
Copy link
Contributor

Handle case when a class without
any handler was published.

Closes #924

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: 924

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Handle case when a class without
any handler was published.

Closes nestjs#924
@kamilmysliwiec kamilmysliwiec merged commit 6d81a84 into nestjs:master Feb 21, 2022
@kamilmysliwiec
Copy link
Member

LGTM

@Sikora00 Sikora00 deleted the bugfix/924 branch February 21, 2022 08:00
@ruslanguns
Copy link

This PR makes it mandatory to setup an event handler per event impl, I thought event impl. shouldn't know about handers. I got an issue since I was triggering an event (which I wasn't expecting to do anything just calling a class in this.apply( new MyEvent() ).

image

If this is an expected behavior I think it would be much better to have a better logging error to know which specific Event was not implemented (with an undefined ID)
image

But in my humble opinion, obviously since I am not master of CQRS, I think an implementation should be decoupled from handlers.

@kamilmysliwiec
Copy link
Member

@Sikora00 can you look into this? We should probably just ignore events that don't have associated event handlers instead of throwing such errors

@Sikora00
Copy link
Contributor Author

Sikora00 commented Mar 3, 2022

@ruslanguns what version of @nestjs/cqrs do you use? I don't see the change introduced with this PR on your screenshots.
I exactly made it optional here :D

@ruslanguns
Copy link

You are right (I was in the wrong version) I feel so embarrassed. Please excuse me!

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.

Cannot read properties of undefined (reading 'id') - possible breaking change for 8.0.1
3 participants