-
Notifications
You must be signed in to change notification settings - Fork 204
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(core): send messages now takes a connection id #491
fix(core): send messages now takes a connection id #491
Conversation
Signed-off-by: Berend Sliedrecht <[email protected]>
public async sendMessage(connection: ConnectionRecord, message: string) { | ||
public async sendMessage(connectionId: string, message: string) { | ||
const connection = await this.connectionService.getById(connectionId) | ||
|
||
const outboundMessage = await this.basicMessageService.send(message, connection) |
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're on it:
Not sure if this is 100% valid code, but the basic message service is still creating the outbound message which in other modules is handled at the module layer
const outboundMessage = await this.basicMessageService.send(message, connection) | |
const basicMessage = await this.basicMessageService.createBasicMessage(content) | |
const outboundMessage = createOutboundMessage(basicMessage, connection) |
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.
Will check it!
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.
@TimoGlastra I implemented your suggested change, however the createMessage
requires a connectionRecord/connectionId to initialize and save a basicMessageRecord
.
See my proposed solution in the latest commit.
Signed-off-by: Berend Sliedrecht <[email protected]>
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.
LGTM
Signed-off-by: Berend Sliedrecht [email protected]