-
-
Couldn't load subscription status.
- Fork 50
feat: add support for compressing Response objects from Fetch API #363
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
base: main
Are you sure you want to change the base?
feat: add support for compressing Response objects from Fetch API #363
Conversation
…cannot Sending something other than buffers of strings in the `payload` happens, but this would break the later `Buffer.byteLength(payload)` Signed-off-by: Bart Riepe <[email protected]>
- Add Response object detection in isCompressiblePayload - Implement convertResponseToStream helper to extract body from Response objects - Handle Response headers and status code preservation in both onSend hook and reply.compress() - Add comprehensive tests for Response object compression scenarios Closes fastify#309 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ypes - Add tests using actual Fastify server with real HTTP requests - Test with both fetch and axios to ensure compatibility - Verify compression works for all supported types: - JSON objects - Plain strings - Buffers - Node.js Readable Streams - Response objects - Raw ReadableStreams - Test edge cases like empty responses and large streams - Ensure clients receive correct decompressed data
- Add got as a third HTTP client to test alongside fetch and axios - Got preserves content-encoding header after decompression, providing an additional verification that compression is working correctly - All 36 tests pass with got, confirming compatibility
When handling Response objects from the Fetch API, the compress middleware now copies headers and status code from the Response to the reply, unless those headers have already been explicitly set on the reply. This provides more intuitive behavior - if someone returns a Response object with specific headers (like content-type, cache-control, etc.), those headers will be preserved in the final response. - Headers already set on the reply take precedence over Response headers - Status code is copied only if reply still has default 200 status - Added comprehensive tests to verify header handling behavior - Updated README to document this behavior
|
|
||
| interface FastifyReply { | ||
| compress(input: Stream | Input): void; | ||
| compress(input: Stream | Input | Response | ReadableStream): void; |
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 needs a type test as well.
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.
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.
Scanning through this, I see many extraneous comments, linting errors, and likely useless tests.
| @@ -0,0 +1,544 @@ | |||
| 'use strict' | |||
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.
How does this relates to this PR?
| }) | ||
| } | ||
|
|
||
| test('It should not compress non-buffer/non-string payloads', async (t) => { |
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.
the test description does not match what is being tested
| t.assert.equal(decompressed, responseBody) | ||
| }) | ||
|
|
||
| test('It should compress Response objects using reply.compress()', async (t) => { |
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.
how is this different from the previous test?
|
|
||
| fastify.get('/', (_request, reply) => { | ||
| reply.header('content-type', 'application/json') | ||
| reply.send(readableStream) |
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.
please just use a ReadableStream object, without passing through Response
| t.assert.equal(decompressed, responseBody) | ||
| }) | ||
|
|
||
| test('It should compress ReadableStream objects using reply.compress()', async (t) => { |
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.
how is this different from the previous test?
After I made the other PR's, I realized there was already an issue open for this, and so I've made an attempt to make a more comprehensive fix to the supported types using Claude Code. It all seems proper to me, and both the new and old tests pass, but still, it's mostly LLM work, so discretion is adviced.
If this is merged, there is no need for #361
Summary
Details
This PR addresses issue #309 by implementing proper handling of Response objects and ReadableStream
objects in fastify-compress.
Key Changes:
Extended payload type support:
isCompressiblePayloadto detect these new typesStream conversion:
convertResponseToStreamhelper that extracts the body from Response objectsReadable.fromWeb()to convert Web Streams to Node.js streamsFocused middleware behavior:
Content-Encoding,Vary)Documentation updates:
reply.compress()examples with Response and ReadableStream usageTypeScript support:
Response | ReadableStreamin compress methodTests added
Breaking Changes
None - this is a backwards-compatible enhancement.
Closes #309
🤖 Generated with Claude Code