-
Notifications
You must be signed in to change notification settings - Fork 126
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: buffer messages from external resource #582
Conversation
I just ran into it as well. I was about open a PR :-). Thanks @nicomoya123. Any chance this can get merged soon? It has been open for a long time. |
? this.deserializer(message, { pattern, channel }) | ||
: JSON.parse(message, this.reviver); | ||
if(this.deserializer){ | ||
parsedMessage = this.deserializer(Buffer.from(message), { pattern, channel }) |
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.
The serializer/deserializer may not be dealing with buffers at all. I think the original code is correct. I think the only change needed is the check above for whether channel is an object.
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.
I might not remember correctly, but I think this is when you set it using the config. Do you have a test case where this is incorrect?
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.
Yes, there are some serialization/deserialization libraries that don't necessarily output buffers or take in buffers. They could be encoding/decoding using strings (inefficient, I know). But then, I guess they would not be setting the eventName to messageBuffer in the config. So this is likely ok.
@davidyaha can this be merged ? |
Thanks @nicomoya123 and sorry for the delay |
תודה !