-
Notifications
You must be signed in to change notification settings - Fork 565
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
permessage-deflate decompression support in websocket #3263
Conversation
Autobahn Test ReportSummary
Details
|
a978d0f
to
b50e9d9
Compare
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
lib/web/websocket/connection.js
Outdated
if (secExtension !== null) { | ||
extensions = parseExtensions(secExtension) | ||
|
||
if (!extensions.has(permessageDeflate)) { |
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.
this is probably wrong
@@ -122,7 +140,7 @@ class ByteParser extends Writable { | |||
return | |||
} | |||
|
|||
if (isContinuationFrame(opcode) && this.#fragments.length === 0) { | |||
if (isContinuationFrame(opcode) && this.#fragments.length === 0 && !this.#info.compressed) { |
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.
this is probably wrong (?)
Please give feedback if you want to have a review |
these 13.x tests are killing me |
@Uzlopak we can land this. The failing tests (to the best of my knowledge) are because we don't compress the messages we send, only decompress. We can add that in at a later date. Will require a queuing system that's not too important for this PR. |
According to the comment 5.18 failed? Is this a regression or a flaky test? |
the comment is always 1 commit behind, it's passing |
https://www.rfc-editor.org/rfc/rfc7692
Famous last words, but I'm looking at the spec and it doesn't seem too difficult to implement.