-
Notifications
You must be signed in to change notification settings - Fork 0
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
Version 2 #1
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This review was done without starting an IDE, so some comments you can ignore if I was wrong
|
||
export abstract class BaseMessageBus implements MessageBus { | ||
protected handlersMap: HandlersMap = {}; | ||
protected middlewareList: MessageBusMiddleware<any>[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Middlewares should match some interface. It can be a single functions where message is passed or classes with ex. handle function which accepts message as a parameter. For now if middleware class will not have a handle method handle
will throw an error about calling undefined function.
if (isMessageHandlerObject(args[0])) { | ||
handler = args[0]; | ||
type = handler.getSupportedType(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe else if? Here you can check if args[0]
is a message.
? args[0].type | ||
: args[0]; | ||
handler = args[1]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can have an else
statement and throw an error inside if type is not a string, or is empty. This will make this code more readable.
} | ||
|
||
if (!isMessageHandler(handler)) { | ||
throw new Error('handler must be a function or an object of type MessageHandlerObject'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify error messages. Handler must be...
. Capital letters
import {isMessageHandlerObject} from './TypeCheckers/isMessageHandlerObject'; | ||
|
||
export abstract class BaseMessageBus implements MessageBus { | ||
protected handlersMap: HandlersMap = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can use a Map
here? It will add more typing to this class.
await middleware.handle(message); | ||
} | ||
|
||
if (!this.handlersMap[type]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use a Map
class in handlersMap
you can assign handlers to const and check this condition.
): [string, MessageHandler<P>] { | ||
const [type, handler] = this.getTypeAndHandlerFromArgs([arg1, arg2]); | ||
if (this.handlersMap[type]) { | ||
throw new Error('There can be only one handler per type in CommandBus'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I cannot handle the same message types in more than one place?
const message2: Message<{lorem: string}> = {type: 'bar', payload: {lorem: 'ipsum'}}; | ||
|
||
class TestMessage implements Message<{dolor: string}> { | ||
readonly type: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type can be initialized here
No description provided.