-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add support for gzip content decoding #951
Conversation
should handle gzip and deflate, just steal this code: https://github.com/mikeal/http-duplex-gzip-client/blob/master/index.js#L28 also, I can't add gzip accept by default before the 3.0 release. it's too big of a change to do in a point release. please add a |
Does that code really support "raw" deflate streams, or only standards-conforming zlib deflate streams? Because I had thought that would fail for servers which send raw streams (IIS). Sure, I'll rename |
@kevinoid i haven't tested it myself but per the node core documentation it should. |
@mikeal See what you think about this version. I've renamed the option to Unfortunately, on my machine (with Node.js v0.10.29) raw deflate encodings result in There's also an argument to be made for not supporting servers that send non-conformant data (since the spec requires the zlib-wrapped encoding), but they are somewhat prevalent and users may not appreciate appeals to the spec. What do you think? |
Support decoding of the gzip Content-Encoding in responses using the zlib module to decode the response before piping it through the request object (and therefore before user-connected pipes or body parsing). Add the boolean option `gzip` to allow users to explicitly request decoding of supported response content and inclusion of appropriate content negotiation headers, if unspecified. This commit favors backwards-compatibility over the increased performance that transparent compression could provide, although it is hoped that a future backwards-incompatible version can make transparent compression the default. Some major tradeoffs of transparent compression are: - It may trigger changes in server behavior and performance (for better or worse) that are unexpected, either due to buggy servers or intermediate network hardware. - The compression is not fully transparent as users who attach to the `data` event of the response (rather than the `data` event of `request`) will get gzipped data rather than uncompressed data. + It is likely a big win for most users (both current and future) who would otherwise be unaware or unable to spend the time to implement content negotiation and decoding. Especially given the prevalence of highly-compressible text content (e.g. JSON and XML) and widespread server support for gzip. Changes since v1: - Rename `decodeContent` option to `gzip` to match option name in 3.0 Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Whoops, overlooked the README. Now updated there as well. |
Hey @mikeal, don't want to pester you, but do you have any thoughts for what to do about deflate support? Should I work on detecting zlib wrapping or not worry about it or drop support for deflate from this PR and save it for later? Can do any of those (or something else), just let me know. |
just drop deflate support for now. |
Sounds good. Dropped the work-in-progress deflate commit from the PR. Let me know if there's anything else that needs attention in the remaining commit. |
Add support for gzip content decoding
Thanks, much appreciated! |
looks like we broke 0.8 :( |
Whoops. My bad. I'll work on it. |
Looks like |
I don't see a really simple way to get I've updated the branch with a commit that uses |
Looks like GitHub doesn't check for branch changes after the pull request has been merged (which makes sense). The commit in question is kevinoid/request@a2da682 . I can submit a separate pull request if it looks acceptable. |
The PR is already merged so you need to make a new PR with only the fix. Thanks. |
Sure thing. Submitted as #960. |
It took me awhile to realise that the uncompressed data is available on the request stream, not on the response stream (received in the |
Sure. The reason is that the response object emitted on the However, the documentation does not currently mention this issue, and I'm sure you aren't the only person to get tripped up by it. I'll work on improving the README to help clarify what is going on. |
Ahh, I see! What kind of stream does request give you then – if it writes and reads?
|
If I understand your question, conceptually you could think of request as a stream.Transform (although it does not inherit from this class) where the data is being transformed by an HTTP server. The (possibly empty) request data is written then transformed by the server then returned as response data. Does that help clarify? |
Ahh, okay. I was trying to do
|
Right. It doesn't currently inherit from stream.Readable and doesn't provide its own However, you can set the encoding with the |
Thanks! Want me to open an issue for this feature request, so you can track it separately from this? On 21 Jul 2014, at 13:17, Kevin Locke notifications@github.com wrote:
|
Sure, that would be great, thanks! |
Thanks! |
More accurately, the Request stream is a Duplex stream, because what you pipe to it does not come our the other end, and what comes out the readable side really doesn't have all that much to do with the input (which is why it isn't just a transform stream). Of course, request predates all the stream base objects in core so it doesn't inherit from either Transform or Duplex :) |
Now I finally know the difference between duplex and transform streams! On 22 Jul 2014, at 15:12, Mikeal Rogers notifications@github.com wrote:
|
Add support for gzip content decoding
My apologies in advance for adding yet another gzip implementation for you to consider, but it appears that work on the alternative solutions (#303, #458, #464, #660) has stalled while interest in a solution is still high (#539, #725, #747) and requires non-trivial effort to implement outside of the library (although it's certainly not horrible).
Unlike the previous solutions, this pull request supports stream-based usage and does not introduce any backwards-incompatible behavior. It is intended to serve as a bridge for the 2.x series until 3.0 is ready for release, at which points its existing gzip implementation can supersede this one.
This commit introduces the
decodeContent
boolean option to allow users to explicitly request decoding of supported response content and inclusion of the appropriate content negotiation header (Accept-Encoding
), if unspecified.Although
deflate
support is also common on servers, this commit does not adddeflate
support due to compatibility issues with non-conformant "raw" deflate encodings and the lack of a compelling reason to support it givengzip
's wide availability.Thanks for considering,
Kevin