feat: support Blob, Response #5641
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying egg with
|
| Latest commit: |
ccee62a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://91f3d7df.egg-cci.pages.dev |
| Branch Preview URL: | https://koa-pick-1893.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5641 +/- ##
==========================================
- Coverage 87.52% 80.44% -7.09%
==========================================
Files 565 534 -31
Lines 11007 10675 -332
Branches 1243 1225 -18
==========================================
- Hits 9634 8587 -1047
- Misses 1291 1942 +651
- Partials 82 146 +64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying egg-v3 with
|
| Latest commit: |
ccee62a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dad6e221.egg-v3.pages.dev |
| Branch Preview URL: | https://koa-pick-1893.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for Blob and Response objects as body types in the Koa framework, addressing issue #5638. The changes introduce handling for web standard API types (Blob, Response, ReadableStream) alongside traditional Node.js streams.
Key changes:
- Added support for
Blob,Response, andReadableStreamas response body types - Renamed internal classes (
Context→KoaContext,Request→KoaRequest,Response→KoaResponse) to avoid naming conflicts with web standardResponse - Migrated from direct stream piping to
Stream.pipelinefor better error handling and stream cleanup
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/koa/src/application.ts |
Updated class names and added stream handling via Stream.pipeline for Blob/Response/ReadableStream |
packages/koa/src/context.ts |
Renamed Context to KoaContext and updated type references |
packages/koa/src/request.ts |
Renamed Request to KoaRequest and updated type references |
packages/koa/src/response.ts |
Added body type handling for Blob/Response/ReadableStream; renamed Response to KoaResponse |
packages/koa/src/index.ts |
Updated exports with deprecation notices for backward compatibility |
packages/koa/src/utils.ts |
Added isStream() utility function for stream type checking |
packages/koa/test/application/respond.test.ts |
Added comprehensive tests for Response, ReadableStream, and pipeline error handling |
packages/koa/test/response/body.test.ts |
Updated test expectations for stream error handling behavior |
packages/koa/test/__snapshots__/index.test.ts.snap |
Updated snapshot with new exported class names |
| this.status = val.status; | ||
| if (setType) this.type = 'bin'; | ||
| const headers = val.headers; | ||
| console.log('headers', headers); |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging to production. This appears to be leftover debugging code that will pollute production logs.
| console.log('headers', headers); |
| describe('when using pipeline for streams', () => { | ||
| it('should handle stream errors when error listener exists', async () => { | ||
| const app = new Koa(); | ||
| const PassThrough = require('stream').PassThrough; |
There was a problem hiding this comment.
Use ES6 import syntax instead of require() to maintain consistency with the rest of the codebase which uses TypeScript imports (e.g., import { PassThrough } from 'node:stream').
|
|
||
| it('should not crash when stream errors and no error listener exists', async () => { | ||
| const app = new Koa(); | ||
| const PassThrough = require('stream').PassThrough; |
There was a problem hiding this comment.
Use ES6 import syntax instead of require() to maintain consistency with the rest of the codebase which uses TypeScript imports (e.g., import { PassThrough } from 'node:stream').
|
|
||
| it('should cleanup streams on client abort', async () => { | ||
| const app = new Koa(); | ||
| const PassThrough = require('stream').PassThrough; |
There was a problem hiding this comment.
Use ES6 import syntax instead of require() to maintain consistency with the rest of the codebase which uses TypeScript imports (e.g., import { PassThrough } from 'node:stream').
| const shouldDestroyOriginal = original && isStream(original); | ||
| if (shouldDestroyOriginal) { | ||
| // Ignore errors during cleanup to prevent unhandled exceptions when destroying the stream | ||
| original.once('error', () => {}); | ||
| destroy(original); | ||
| } |
There was a problem hiding this comment.
[nitpick] The variable shouldDestroyOriginal is only used once immediately after declaration. Consider simplifying by using the condition directly in the if statement: if (original && isStream(original)).
close #5638