-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(ws): introduce message parser for ws adapter #14127
base: 11.0.0
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build c2aa204b-3107-4901-82ec-903bb6861111Details
💛 - Coveralls |
@@ -138,7 +142,10 @@ export class WsAdapter extends AbstractWsAdapter { | |||
transform: (data: any) => Observable<any>, | |||
): Observable<any> { | |||
try { | |||
const message = JSON.parse(buffer.data); | |||
const message = this.messagePreprocessor(JSON.parse(buffer.data)); |
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 wonder if we should move the "JSON.parse" call to the preprocessor, and just call it parser instead?
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.
It's a great idea.
Additionally, should we consider supporting configuration in the constructor, like this:
new WsAdapter(app, {
messageParser: (data) => ...
})
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.
sure, sounds good!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #14027
What is the new behavior?
Add a
setMessagePreprocessor
method to thewsAdapter
to customize how messages are preprocessed.Does this PR introduce a breaking change?
Other information