fix: ws errors should conform to jsonrpc spec #2505
Conversation
Signed-off-by: Ted Yeh <[email protected]>
ted-yeh-vc
requested review from
AlfredoG87,
ebadiere,
georgi-l95,
Ivo-Yankov and
a team
as code owners
|
I could be in the wrong, but it seems unnecessary to push an error to the client before closing the connection. The connection closing has its own code and message. Was there a particular reason the separate message is being sent? |
Hi @ted-yeh-vc thanks for poiniting it out! You're right the closing connection has its own code and message, when I pushed in the update I thought it could make it a little more informative for some users by informing them the error before closing the connection. Just a little slight touch on the UX. I will discuss with the team and get back to you later! |
There was a problem hiding this comment.
So, I discussed with my team and we decided to keep the current layout with the extra piece of message sent back to the client. Also, we aggreed that your approach to make the error conform to jsonrpc spec is necessary, and we want the errors to have a consistent structure. Please find the change suggestion below for the new structure. Thanks a lot for the time and effort!
Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: ted-yeh-vc <[email protected]>
Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: ted-yeh-vc <[email protected]>
Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: ted-yeh-vc <[email protected]>
|
Fantastic. Thanks for the quick review. Always happy to help! |
|
|
@ted-yeh-vc looks like the first commit didn't get verified. Please fix it for the PR to be mergable. |
Description:
This PR fixes the error returned from websockets being closed to be conformant to json rpc spec re: errors.
https://www.jsonrpc.org/specification#error_object
Related issue(s):
Fixes #2362
Notes for reviewer:
Checklist