Support for STOMP inside SockJS WebSocket messages
Categories
(DevTools :: Netmonitor, enhancement, P3)
Tracking
(firefox83 fixed)
| Tracking | Status | |
|---|---|---|
| firefox83 | --- | fixed |
People
(Reporter: rockingskier, Assigned: rockingskier)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
SOCK+STOMP.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:80.0) Gecko/20100101 Firefox/80.0
Steps to reproduce:
The WebSocket payload preview is able to parse both SockJS and STOMP messages separately.
A common use case it to combine STOMP and SockJS: https://stomp-js.github.io/guide/stompjs/rx-stomp/ng2-stompjs/using-stomp-with-sockjs.html
The payload preview should be able to parse and nicely display STOMP+SockJS payloads.
Actual results:
The SockJS message is parsed however the inner STOMP frame is left as a string.
Expected results:
Both the SockJS message and the inner STOMP frames should be parsed and displayed in a user friendly format.
|
||
Comment 1•4 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
|
||
Comment 2•4 years ago
|
||
Thanks for reporting!
|
Assignee | |
Comment 3•4 years ago
|
||
I am happy to take on this, it follow on from my previous work (https://bugzilla.mozilla.org/show_bug.cgi?id=1606626).
I'm mainly wondering where is best to put this additional parsing.
Option one:
In MessagePayload (https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/messages/MessagePayload.js#164-165).
if (sockJSPayload.type === "message") {
// Do STOMP parsing to sockJSPayload.data
}
Option two:
In the SockJS parser (https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/messages/parsers/sockjs/index.js#42-45)
Add logic to the type case.
case "a":
case "m":
// Do STOMP parsing to `payload`
return { type: "message", data: payload };
I'm inclined to say option one as I don't like the idea of pulling the STOMP parser into the SockJS parser. MessgePayload it where everything comes together currently so it makes sense there.
I feel like I've talked myself in to option one now but I'll leave this here in case anyone has an alternative opinion.
Thanks for working on this!
Option #1 looks good to me too.
Honza
|
|
Assignee | |
Comment 5•4 years ago
|
||
|
|
Assignee | |
Comment 6•4 years ago
|
||
|
|
Assignee | |
Comment 7•4 years ago
|
||
I had some computer issues which meant I've somehow created a second patch. I'm not sure how to fix this but the second patch is the most up to date one.
As per the previous work you can use this app to test against. The app sends JSON and returns SockJS, both should format correctly now.
I realised while working on this that the SockJS parser is only used on responses, this is because the request payload is "just" JSON. As such I've added the STOMP parsing to the JSON logic too.
I thought about sufixing the panel titles with "+ STOMP" ("JSON + STOMP", "SockJS + STOMP") but felt the code branches added weren't worth the small payoff.
Finally, the test are mainly copy/pasted versions of each other with minor changes. I tried to DRY them out but they ended up more compliated and I felt the duplication was actually a lot "cleaner" to some extent. I'm open to suggestions for alternatives.
(In reply to Ben D (:rockingskier) from comment #7)
I had some computer issues which meant I've somehow created a second patch. I'm not sure how to fix this but the second patch is the most up to date one.
I see, no worries, I removed the first one.
I'll review the second one in Phab
Thanks for working on this!
Honza
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/25acca3192a3 Parse STOMP within SockJS WebSocket messages. r=Honza,bomsy
|
||
Comment 10•4 years ago
|
||
| bugherder | ||
Description
•