fix(server): do not wait for response.write for response.end
#2383
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis update refactors stream and response writing logic across several packages to use new utility functions for safe, backpressure-aware writing and controlled stream termination. It addresses issues with Express's Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExpressApp
participant CompressionMiddleware
participant ServerAdapter
participant Response
Client->>ExpressApp: POST /my-path (large body)
ExpressApp->>CompressionMiddleware: Pass request
CompressionMiddleware->>ServerAdapter: Pass request/response
ServerAdapter->>Response: Write response body using safeWrite
alt write() returns true
ServerAdapter->>Response: end() immediately
else write() returns false
ServerAdapter->>Response: Wait for 'drain', then end()
end
Response-->>Client: Compressed JSON response
Assessment against linked issues
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (17)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@whatwg-node/fetch |
0.10.7-alpha-20250509160231-1e764d6503d33d7a355917720b97a4de05039165 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/node-fetch |
0.7.19-alpha-20250509160231-1e764d6503d33d7a355917720b97a4de05039165 |
npm ↗︎ unpkg ↗︎ |
@whatwg-node/server |
0.10.6-alpha-20250509160231-1e764d6503d33d7a355917720b97a4de05039165 |
npm ↗︎ unpkg ↗︎ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/whole-planes-mate.md (1)
1-11: Well-documented changeset with minor formatting improvements neededThe changeset correctly describes the problem and solution. Consider these small improvements:
- Format the bare URL as a markdown link
- Fix the usage of "workaround" as a verb
Some implementations like `compression` npm package do not implement `response.write(data, callback)` signature, but whatwg-node/server waits for it to finish the response stream. It is actually a bug in `compression` package; -https://github.com/expressjs/compression/blob/master/index.js#L99 -But since it is a common mistake, we prefer to workaround this on our end. +[See code here](https://github.com/expressjs/compression/blob/master/index.js#L99) +But since it is a common mistake, we prefer to work around this on our end.🧰 Tools
🪛 LanguageTool
[grammar] ~11-~11: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end.(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
10-10: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
.changeset/whole-planes-mate.md(1 hunks)packages/server/package.json(1 hunks)packages/server/src/utils.ts(1 hunks)packages/server/test/reproductions.spec.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[grammar] ~11-~11: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end.
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
.changeset/whole-planes-mate.md
10-10: Bare URL used
null
(MD034, no-bare-urls)
🪛 Biome (1.9.4)
packages/server/test/reproductions.spec.ts
[error] 84-84: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / deno
- GitHub Check: unit / bun
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 20
- GitHub Check: server (undici)
- GitHub Check: type check
- GitHub Check: server (ponyfill)
- GitHub Check: esm
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: unit / node 18
- GitHub Check: server (native)
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: prettier
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
packages/server/package.json (1)
45-45: Dependencies added correctly for testing with compressionThe addition of
compressionand@types/compressionas devDependencies is appropriate for testing the fix. These dependencies will allow the test suite to verify that the server adapter works correctly with Express's compression middleware.Also applies to: 49-49
packages/server/src/utils.ts (1)
358-359: Good fix for compatibility with middleware packagesThis change addresses the core issue by calling
endResponseimmediately afterserverResponse.write, rather than waiting for a write callback that may never be invoked. This ensures compatibility with packages likecompressionthat don't properly implement theresponse.write(data, callback)signature.The change is minimal and focused only on the optimized node-fetch buffer path, which is exactly what was needed to fix the issue.
packages/server/test/reproductions.spec.ts (1)
3-3: Import added for the compression middlewareThe import statement for the compression middleware is correctly added to support the new test case.
✅
|
✅
|
✅
|
✅
|
✅
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/whole-planes-mate.md (4)
5-6: Enhance clarity and punctuation in the problem description.Consider refining the sentence for readability and adding a comma for correct apposition:
- Some implementations like `compression` npm package do not implement `response.write(data, callback)` signature, but whatwg-node/server waits for it to finish the response stream. + Some implementations, such as the `compression` npm package, do not implement the `response.write(data, callback)` signature, yet `@whatwg-node/server` currently waits for it before ending the response stream.
6-7: Correct verb form for consistency.Replace “hangs” with the infinitive “hang” and streamline the clause:
- Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold. + This can cause the response stream to hang if `compression` takes over when streaming large responses.
8-10: Fix punctuation, verb usage, and format the URL as a markdown link.
- Use a colon instead of a semicolon.
- Change “workaround” (noun) to “work around” (verb).
- Embed the bare URL in markdown for better readability.
- It is actually a bug in `compression` package; - https://github.com/expressjs/compression/blob/master/index.js#L99 - But since it is a common mistake, we prefer to workaround this on our end. + This is actually a bug in the `compression` package: + [See the implementation here](https://github.com/expressjs/compression/blob/master/index.js#L99). + However, because this mistake is common, we prefer to work around it on our end.🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null(MD034, no-bare-urls)
12-12: Clarify the scope and context of the change.Explicitly reference that this behavior is introduced by the patch:
- So now the server adapter calls `response.end` immediately after `response.write` for static responses. + This patch makes the server adapter call `response.end` immediately after `response.write` for static responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/whole-planes-mate.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...
(NOUN_VERB_CONFUSION)
🪛 markdownlint-cli2 (0.17.2)
.changeset/whole-planes-mate.md
9-9: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: unit / node 23
- GitHub Check: e2e / azure-function
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / bun
- GitHub Check: unit / node 20
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: prettier
- GitHub Check: esm
- GitHub Check: lint
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: server (native)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/whole-planes-mate.md (1)
1-3: Frontmatter header follows the standard Changeset format and does not require updates.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/utils.ts (1)
5-11: Import looks good but consider keeping the list alphabetised
handleMaybePromiseis correctly added to the helper imports.
(Optional) To keep imports tidy and minimise merge conflicts down the line, you may want to alphabetise this import list (e.g. movehandleMaybePromiseafterfakePromise).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/server/src/utils.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/utils.ts (1)
packages/promise-helpers/src/index.ts (1)
handleMaybePromise(29-42)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: unit / deno
- GitHub Check: esm
- GitHub Check: e2e / azure-function
- GitHub Check: unit / node 23
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: lint
- GitHub Check: server (native)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: prettier
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (2)
packages/server/src/utils.ts (2)
366-369: 👍 Good switch to promise-based flowReplacing the callback form of
writewithhandleMaybePromise → safeWriteremoves the dependency on middleware correctly passing a callback, fixing the compression issue without changing observable behaviour for well-behaved implementations.
384-386: 👍 Consistent treatment for Uint8Array bodiesThe same promise-based pattern is applied to
Uint8Arraybodies, keeping the control flow uniform across buffer types.
| function safeWrite(chunk: any, serverResponse: NodeResponse) { | ||
| // @ts-expect-error http and http2 writes are actually compatible | ||
| const result = serverResponse.write(chunk); | ||
| if (!result) { | ||
| return new Promise(resolve => serverResponse.once('drain', resolve)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
safeWrite can still hang when the socket closes before drain fires
safeWrite resolves only on the 'drain' event. If serverResponse.write returns false and the connection is closed or errors out before the internal buffer is drained, 'drain' never fires – the returned promise remains pending and endResponse is never called, resurrecting the very hang this PR tries to eliminate.
Recommended refinement:
-function safeWrite(chunk: any, serverResponse: NodeResponse) {
- // @ts-expect-error http and http2 writes are actually compatible
- const result = serverResponse.write(chunk);
- if (!result) {
- return new Promise(resolve => serverResponse.once('drain', resolve));
- }
-}
+function safeWrite(
+ chunk: Buffer | Uint8Array | string,
+ serverResponse: NodeResponse,
+): Promise<void> | void {
+ // @ts-expect-error http and http2 writes are actually compatible
+ const result = serverResponse.write(chunk);
+ if (!result) {
+ return new Promise(resolve => {
+ const done = () => {
+ serverResponse.removeListener('error', done);
+ serverResponse.removeListener('close', done);
+ resolve();
+ };
+ serverResponse.once('drain', done);
+ serverResponse.once('error', done);
+ serverResponse.once('close', done);
+ });
+ }
+}Benefits
• Guarantees promise resolution on 'drain', 'error', or 'close', preventing hangs.
• Narrows chunk’s type and adds an explicit return type for better type-safety.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function safeWrite(chunk: any, serverResponse: NodeResponse) { | |
| // @ts-expect-error http and http2 writes are actually compatible | |
| const result = serverResponse.write(chunk); | |
| if (!result) { | |
| return new Promise(resolve => serverResponse.once('drain', resolve)); | |
| } | |
| } | |
| function safeWrite( | |
| chunk: Buffer | Uint8Array | string, | |
| serverResponse: NodeResponse, | |
| ): Promise<void> | void { | |
| // @ts-expect-error http and http2 writes are actually compatible | |
| const result = serverResponse.write(chunk); | |
| if (!result) { | |
| return new Promise(resolve => { | |
| const done = () => { | |
| serverResponse.removeListener('error', done); | |
| serverResponse.removeListener('close', done); | |
| resolve(); | |
| }; | |
| serverResponse.once('drain', done); | |
| serverResponse.once('error', done); | |
| serverResponse.once('close', done); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents (early access)
In packages/server/src/utils.ts around lines 321 to 327, the safeWrite function currently only resolves its promise on the 'drain' event, which can cause it to hang if the socket closes or errors before 'drain' fires. To fix this, modify safeWrite to also listen for 'error' and 'close' events on serverResponse and resolve the promise when any of these events occur. Additionally, narrow the type of the chunk parameter and add an explicit return type to improve type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.changeset/whole-planes-mate.md (2)
5-6: Clarify hanging response description
The current wording is a bit wordy and contains a minor grammatical slip (“hangs” → “hang”), which could be streamlined for readability. Consider revising to something like:- Some implementations like `compression` npm package do not implement `response.write(data, callback)` signature, but whatwg-node/server waits for it to finish the response stream. - Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold. + Some middleware, such as the `compression` package, does not support the `response.write(data, callback)` signature. @whatwg-node/server currently waits for this callback before ending the response, which can cause the stream to hang on larger payloads.
8-10: Fix verb usage and framework naming
The phrase “workaround this” should be split into two words (“work around this”) when used as a verb. Also, use the official framework name “Express.js” when referring to the middleware. For example:-It is actually a bug in `compression` package; -[expressjs/compression#46](https://github.com/expressjs/compression/issues/46) -But since it is a common mistake, we prefer to workaround this on our end. +This is a bug in the Express.js `compression` middleware (see [expressjs/compression#46](https://github.com/expressjs/compression/issues/46)). Since this mistake is common, we prefer to work around it on our end.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug incompressionpackage; [expressjs/compression#46](https://github.com/expr...(NODE_JS)
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...(NOUN_VERB_CONFUSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/whole-planes-mate.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[uncategorized] ~9-~9: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug in compression package; [expressjs/compression#46](https://github.com/expr...
(NODE_JS)
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...
(NOUN_VERB_CONFUSION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.changeset/whole-planes-mate.md (1)
12-12: Changelog entry clearly summarizes the fix
The final sentence concisely describes the new behavior of callingresponse.endimmediately afterresponse.write. LGTM!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.changeset/whole-planes-mate.md (3)
5-6: Clarify and improve wording for readability
The current sentences are a bit convoluted; consider merging and refining to clearly explain the issue in one coherent statement.- Some implementations like `compression` npm package do not implement `response.write(data, callback)` signature, but whatwg-node/server waits for it to finish the response stream. - Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold. + Some middleware packages (e.g., `compression`) do not implement the `response.write(data, callback)` signature. + The `@whatwg-node/server` adapter currently waits for the write callback to complete, which can cause the response stream to hang when the middleware intercepts larger payloads.
8-10: Fix punctuation and verb usage
Adjust punctuation for clarity, add the missing article before “compression” and correct the verb form “work around.”- It is actually a bug in `compression` package; - [expressjs/compression#46](https://github.com/expressjs/compression/issues/46) - But since it is a common mistake, we prefer to workaround this on our end. + This is actually a bug in the `compression` package (expressjs/compression#46). + However, since this mistake is common, we prefer to work around it on our end.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug incompressionpackage; [expressjs/compression#46](https://github.com/expr...(NODE_JS)
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...(NOUN_VERB_CONFUSION)
12-12: Enhance phrasing for consistency
Replace “So now” with “As a result” to improve flow and match formal changelog tone.- So now the server adapter calls `response.end` immediately after `response.write` for static responses. + As a result, the server adapter now calls `response.end` immediately after `response.write` for static responses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
.changeset/whole-planes-mate.md(1 hunks)packages/server/package.json(1 hunks)packages/server/src/utils.ts(6 hunks)packages/server/test/reproductions.spec.ts(2 hunks)packages/server/test/test-server.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/server/test/test-server.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/server/package.json
- packages/server/test/reproductions.spec.ts
- packages/server/src/utils.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[uncategorized] ~9-~9: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug in compression package; [expressjs/compression#46](https://github.com/expr...
(NODE_JS)
[grammar] ~10-~10: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...
(NOUN_VERB_CONFUSION)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / node 23
- GitHub Check: unit / node 22
- GitHub Check: unit / deno
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: alpha / snapshot
- GitHub Check: unit / node 18
- GitHub Check: server (undici)
- GitHub Check: esm
- GitHub Check: server (ponyfill)
- GitHub Check: type check
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: lint
- GitHub Check: server (native)
- GitHub Check: prettier
- GitHub Check: node-fetch (noConsumeBody)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.changeset/whole-planes-mate.md (2)
7-9: Fix grammatical error in hanging description.
The phrase "causes the response stream hangs" is grammatically incorrect and can be clearer. Consider rephrasing:- Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold. + This can cause the response stream to hang when the compression middleware takes over for larger response payloads.
10-13: Correct framework naming and verb usage.
- Change the semicolon after
compressionpackage to a period.- Capitalize the framework name as
Express.js.- Use the verb form "work around" instead of the noun "workaround".
- It is actually a bug in `compression` package; - [expressjs/compression#46](https://github.com/expressjs/compression/issues/46) - But since it is a common mistake, we prefer to workaround this on our end. + It is actually a bug in the `compression` package. + [Express.js/compression#46](https://github.com/expressjs/compression/issues/46) + But since it is a common mistake, we prefer to work around this on our end.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug incompressionpackage; [expressjs/compression#46](https://github.com/expr...(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...(NOUN_VERB_CONFUSION)
packages/node-fetch/src/FormData.ts (1)
132-155: Node-specificBufferusage ties the ponyfill to Node and adds unnecessary conversionsAll header/body fragments are wrapped in
Buffer.from(...), but the public contract (PonyfillReadableStream<Uint8Array>) only promisesUint8Array. While Buffer is a subclass, relying on it:
- Couples the ponyfill to Node (e.g. Deno or Cloudflare Workers won’t have
Buffer).- Performs an extra allocation (
Buffer.from(string)does a copy) whereTextEncoderwould be sufficient.A minimal, environment-agnostic change would be:
- controller.enqueue(Buffer.from(`--${boundary}\r\n`)); + controller.enqueue(new TextEncoder().encode(`--${boundary}\r\n`));The same applies to every other
controller.enqueue(Buffer.from(...))call.This keeps compatibility while reducing Node-only surface area and avoids the implicit copy that
Buffer.fromincurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/whole-planes-mate.md(1 hunks)packages/node-fetch/src/FormData.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug in compression package; [expressjs/compression#46](https://github.com/expr...
(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. So now the server ada...
(NOUN_VERB_CONFUSION)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / deno
- GitHub Check: unit / node 23
- GitHub Check: esm
- GitHub Check: unit / bun
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: type check
- GitHub Check: server (ponyfill)
- GitHub Check: server (native)
- GitHub Check: prettier
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: lint
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (2)
.changeset/whole-planes-mate.md (2)
1-5: Changelog frontmatter format is correct.
14-15: Changelog description of workaround is clear.
The summary of the workaround succinctly describes the change to callresponse.endimmediately afterresponse.writefor static responses. No issues found.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/node-fetch/src/FormData.ts (1)
99-99:⚠️ Potential issueIncorrect type for
currentAsyncIteratorwill break type-safetyThe
currentAsyncIteratoris declared with typeAsyncIterator<[string, FormDataEntryValue]>, but on line 127 it's assignedentryStream[Symbol.asyncIterator]()which yieldsUint8ArrayorBufferchunks, not key-value pairs. This type mismatch will cause issues when using the iterator's values.- let currentAsyncIterator: AsyncIterator<[string, FormDataEntryValue]> | undefined; + let currentAsyncIterator: AsyncIterator<Uint8Array | Buffer> | undefined;
🧹 Nitpick comments (5)
.changeset/whole-planes-mate.md (2)
10-12: Grammar and style improvements
- Capitalize the framework name: use
Express.jsinstead ofexpressjs.- Use the verb phrase
work aroundinstead of the nounworkaround.- Replace the semicolon after “package” with a period for better readability.
Example suggestion:
-It is actually a bug in `compression` package; [expressjs/compression#46](...) +It is actually a bug in the `compression` package. [Express.js/compression#46](...) -But since it is a common mistake, we prefer to workaround this on our end. +But since it is a common mistake, we prefer to work around this on our end.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug incompressionpackage; [expressjs/compression#46](https://github.com/expr...(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. Now after calling `re...(NOUN_VERB_CONFUSION)
14-18: Refine compound sentences and punctuation
- Add a comma before “but” when linking independent clauses.
- Replace the semicolon with a period or comma to improve flow.
- Insert a comma before “and” in the compound sentence for consistency.
Suggested revision:
-Now after calling `response.write`, it no longer uses callback but first it checks the result; +Now, after calling `response.write`, it no longer uses a callback, but first it checks the result. -if it is `true`, it means stream is drained and we can call `response.end` immediately. +If the result is `true`, the stream is drained, and we can call `response.end` immediately. -else if it is `false`, it means the stream is not drained yet, so we can wait for the `drain` event to call `response.end`. +Otherwise, if it is `false`, the stream is not yet drained, so we wait for the `drain` event before calling `response.end`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ponse.write, it no longer uses callback but first it checks the result; if it is...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...it istrue, it means stream is drained and we can callresponse.endimmediately....(COMMA_COMPOUND_SENTENCE)
packages/node-fetch/src/FormData.ts (3)
132-158: Improve performance with single pass processingThe current implementation alternates between processing form boundaries and streaming file contents, which can be inefficient for large forms with many files. Consider implementing a queue-based approach that batches headers and boundaries.
The current implementation is functionally correct, but for forms with many file entries, the constant switching between iterators might introduce overhead. A more efficient approach could buffer headers and process entries more contiguously.
155-158: Enhance error handling in cancel methodThe
cancelmethod should handle errors that might occur when callingreturnon iterators.cancel: err => { - entriesIterator?.return?.(err); - currentAsyncIterator?.return?.(err); + try { + entriesIterator?.return?.(err); + } catch (error) { + console.error('Error closing entriesIterator:', error); + } + try { + currentAsyncIterator?.return?.(err); + } catch (error) { + console.error('Error closing currentAsyncIterator:', error); + } },
93-96: Consider using a more unique boundary stringThe default boundary string
'---'is very simple and might appear in the content. Consider using a more unique default.export function getStreamFromFormData( formData: FormData, - boundary = '---', + boundary = `----FormBoundary${Math.random().toString(36).substring(2)}`, ): PonyfillReadableStream<Uint8Array> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.changeset/whole-planes-mate.md(1 hunks)packages/node-fetch/src/FormData.ts(1 hunks)packages/node-fetch/src/TransformStream.ts(2 hunks)packages/node-fetch/src/WritableStream.ts(3 hunks)packages/node-fetch/src/fetchNodeHttp.ts(4 hunks)packages/node-fetch/src/utils.ts(1 hunks)packages/server/package.json(1 hunks)packages/server/src/utils.ts(6 hunks)packages/server/test/reproductions.spec.ts(2 hunks)packages/server/test/test-server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/server/package.json
- packages/node-fetch/src/TransformStream.ts
- packages/node-fetch/src/utils.ts
- packages/node-fetch/src/fetchNodeHttp.ts
- packages/server/test/test-server.ts
- packages/node-fetch/src/WritableStream.ts
- packages/server/src/utils.ts
- packages/server/test/reproductions.spec.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug in compression package; [expressjs/compression#46](https://github.com/expr...
(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. Now after calling `re...
(NOUN_VERB_CONFUSION)
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ponse.write, it no longer uses callback but first it checks the result; if it is ...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...it is true, it means stream is drained and we can call response.end immediately....
(COMMA_COMPOUND_SENTENCE)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: e2e / cloudflare-modules
- GitHub Check: e2e / cloudflare-workers
- GitHub Check: e2e / azure-function
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / node 23
- GitHub Check: unit / deno
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: unit / bun
- GitHub Check: server (undici)
- GitHub Check: unit / node 18
- GitHub Check: server (ponyfill)
- GitHub Check: esm
- GitHub Check: alpha / snapshot
- GitHub Check: prettier
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: type check
- GitHub Check: server (native)
- GitHub Check: node-fetch (noConsumeBody)
- GitHub Check: lint
🔇 Additional comments (2)
.changeset/whole-planes-mate.md (2)
1-5: Verify changeset frontmatter formatThe YAML frontmatter correctly lists each package with the
patchrelease type, matching the repository’s changeset conventions. No changes needed here.
7-9: Clarify description of hanging streamThe explanation clearly describes why the response stream can hang when using
compressionwith larger payloads. It concisely sets up the motivation for the workaround.
| if (value) { | ||
| return controller.enqueue(value); | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type checking for stream values
After fixing the currentAsyncIterator type, we should ensure that the values being enqueued are of the expected type.
if (value) {
- return controller.enqueue(value);
+ return controller.enqueue(Buffer.isBuffer(value) ? value : Buffer.from(value));
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (value) { | |
| return controller.enqueue(value); | |
| } else { | |
| if (value) { | |
| return controller.enqueue(Buffer.isBuffer(value) | |
| ? value | |
| : Buffer.from(value)); | |
| } else { |
🤖 Prompt for AI Agents (early access)
In packages/node-fetch/src/FormData.ts around lines 146 to 148, the code
enqueues a value without verifying its type. To fix this, add a type check
before enqueuing to ensure the value matches the expected stream type. This
prevents runtime errors by only enqueuing valid stream values.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/node-fetch/src/FormData.ts (1)
147-148: Add type checking for stream valuesWhen enqueueing values from the async iterator, there's no type verification which could lead to runtime errors if unexpected data comes through the stream.
if (value) { - return controller.enqueue(value); + return controller.enqueue(Buffer.isBuffer(value) ? value : Buffer.from(value)); } else {
🧹 Nitpick comments (1)
packages/node-fetch/src/FormData.ts (1)
127-128: Remove@ts-expect-errorby using proper typingThere's a type error being suppressed with
@ts-expect-error. Consider fixing the underlying type issue rather than suppressing it.- const entryStream = blobOrString.stream(); - // @ts-expect-error - ReadableStream is async iterable - currentAsyncIterator = entryStream[Symbol.asyncIterator](); + const entryStream: ReadableStream<Uint8Array> = blobOrString.stream(); + currentAsyncIterator = entryStream[Symbol.asyncIterator]();Alternatively, you could create a type guard or assertion function to safely access the asyncIterator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/node-fetch/src/FormData.ts(1 hunks)packages/promise-helpers/src/index.ts(1 hunks)packages/server/src/createServerAdapter.ts(1 hunks)packages/server/src/utils.ts(7 hunks)test.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/promise-helpers/src/index.ts (3)
packages/node-fetch/src/utils.ts (1)
fakePromise(30-30)packages/server/src/uwebsockets.ts (1)
fakePromise(277-277)packages/server/src/utils.ts (1)
isPromise(14-14)
🔇 Additional comments (8)
packages/promise-helpers/src/index.ts (3)
35-42: Optimization for the case with finallyFactory looks good.The code now properly distinguishes between cases where a
finallyFactoryis provided, retaining the original implementation for those cases while enabling a more optimized path for cases without the finally handler.
43-56: Performance optimization for promise handling.This implementation optimizes handling of different promise types by:
- Directly extracting values from fake promises without chaining
- Properly handling fake rejected promises
- Only creating promise chains when actually needed
- Using synchronous execution paths when possible
This should reduce unnecessary promise creation and improve performance in the common case.
57-64: Good error handling strategy.The implementation correctly handles errors differently based on whether an error factory is provided:
- When no error factory exists, it allows errors to propagate naturally
- When an error factory exists, it catches synchronous errors and redirects to the error factory
This maintains consistent behavior with the original implementation while optimizing performance.
packages/node-fetch/src/FormData.ts (4)
97-99: Type issue fixed forcurrentAsyncIteratorThe type definition for
currentAsyncIteratorhas been correctly updated toAsyncIterator<Uint8Array> | undefinedwhich properly reflects the actual data type yielded by blob streams. This resolves the type safety concerns mentioned in previous reviews.
156-159: Good addition ofcancelmethod for proper resource cleanupThe addition of the
cancelmethod with proper cleanup of both iterators is an excellent practice for resource management in stream implementations. The use of optional chaining (?.) helps avoid errors if the methods don't exist.
101-132: Well-structuredhandleNextEntryfunctionThis function effectively encapsulates the logic for processing FormData entries, making the code more maintainable. It correctly handles both string and blob values, generating appropriate multipart form data boundaries and headers.
133-155: Improved stream handling with async processingThe refactored implementation properly handles asynchronous streaming of form data entries with better backpressure management. The conditional logic in the
pullmethod ensures entries are processed one at a time, moving to the next entry only after the current one is fully processed.packages/server/src/createServerAdapter.ts (1)
280-286:✅ Verification successful
Implementation improved to handle middleware that doesn't properly implement
response.writecallbacks.The refactored approach correctly passes the
nodeResponsedirectly tonormalizeNodeRequestinstead of using a weakmap. This aligns with the PR objective to fix hanging responses when using middleware like thecompressionpackage that doesn't properly implement theresponse.write(data, callback)signature.Let's verify how the new implementation handles Node.js response streams:
🏁 Script executed:
#!/bin/bash # Description: Check the implementation details of normalizeNodeRequest function # to understand how it now handles the nodeResponse parameter rg -A 10 "normalizeNodeRequest" --glob "packages/server/src/utils.ts"Length of output: 802
🏁 Script executed:
#!/bin/bash # Inspect the full implementation of normalizeNodeRequest echo "---- normalizeNodeRequest (with context) ----" rg -n "export function normalizeNodeRequest" -A 200 --glob packages/server/src/utils.ts echo echo "---- All references to nodeResponse in utils.ts ----" rg -n "nodeResponse" --glob packages/server/src/utils.ts echo echo "---- Any WeakMap usage in the repo ----" rg -i -n "weakmap" -C 3Length of output: 17558
🏁 Script executed:
#!/bin/bash # Check for any WeakMap usage in createServerAdapter.ts echo "---- WeakMap usage in createServerAdapter.ts ----" rg -n "WeakMap" -C 5 --glob packages/server/src/createServerAdapter.ts || echo "No WeakMap usages found." echo # Locate handleRequest invocation(s) to see how nodeResponse flows echo "---- handleRequest calls in createServerAdapter.ts ----" rg -n "handleRequest" -C 5 --glob packages/server/src/createServerAdapter.ts || echo "No handleRequest calls found." echo # Check for decompressedResponseMap references in this file echo "---- decompressedResponseMap references ----" rg -n "decompressedResponseMap" -C 5 --glob packages/server/src/createServerAdapter.ts || echo "No decompressedResponseMap references found."Length of output: 10964
Pass
nodeResponseintonormalizeNodeRequestto bind abort signals
- File:
packages/server/src/createServerAdapter.ts- Lines: 281–286
// Ensure `waitUntil` is available in the server context if (!serverContext.waitUntil) { serverContext.waitUntil = waitUntil; } - const request = normalizeNodeRequest(nodeRequest, fetchAPI); + const request = normalizeNodeRequest(nodeRequest, fetchAPI, nodeResponse); return handleRequest(request, serverContext);This change forwards the Node.js
nodeResponsedirectly intonormalizeNodeRequest, which now registerserror,close, andfinishlisteners on the response stream to abort the request body’sAbortControllerwhen the response ends unexpectedly. It prevents hanging requests when middleware (e.g., compression) doesn’t invoke theresponse.write(data, callback)callback correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/node-fetch/src/FormData.ts (1)
147-149: Add type checking for stream valuesThere's no validation that
valueis of the expected type before enqueueing. This was noted in a previous review comment but hasn't been addressed.if (value) { - return controller.enqueue(value); + return controller.enqueue(Buffer.isBuffer(value) ? value : Buffer.from(value)); } else {
🧹 Nitpick comments (4)
packages/node-fetch/src/FormData.ts (2)
126-128: Consider removing the TypeScript error suppression commentThe
@ts-expect-errorcomment suggests there's a typing issue with the ReadableStream's async iterable interface. Consider addressing this more directly with appropriate type declarations or interfaces.- // @ts-expect-error - ReadableStream is async iterable - currentAsyncIterator = entryStream[Symbol.asyncIterator](); + // ReadableStream's AsyncIterator yields Uint8Array chunks + currentAsyncIterator = entryStream[Symbol.asyncIterator]() as AsyncIterator<Uint8Array>;
133-155: Consider adding explicit error handlingThe implementation lacks explicit error handling in the pull callback. Consider adding try/catch blocks or error handling for the promises to prevent stream crashes due to errors in blob processing.
pull: controller => { if (!sentInitialHeader) { sentInitialHeader = true; return controller.enqueue(Buffer.from(`--${boundary}\r\n`)); } if (currentAsyncIterator) { - return currentAsyncIterator.next().then(({ done, value }) => { - if (done) { - currentAsyncIterator = undefined; - } - if (value) { - return controller.enqueue(value); - } else { - return handleNextEntry(controller); - } - }); + return currentAsyncIterator.next() + .then(({ done, value }) => { + if (done) { + currentAsyncIterator = undefined; + } + if (value) { + return controller.enqueue(Buffer.isBuffer(value) ? value : Buffer.from(value)); + } else { + return handleNextEntry(controller); + } + }) + .catch(error => { + // Log the error or transform it to a more user-friendly format + console.error('Error processing form data stream:', error); + controller.error(error); + }); } return handleNextEntry(controller); },.changeset/whole-planes-mate.md (2)
7-12: Improve clarity and correct grammar in description.Lines 7–12 contain some grammatical issues and can be made clearer. Suggested diff:
- Some implementations like `compression` npm package do not implement `response.write(data, callback)` signature, but whatwg-node/server waits for it to finish the response stream. - Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold. - It is actually a bug in `compression` package; [expressjs/compression#46](https://github.com/expressjs/compression/issues/46) - But since it is a common mistake, we prefer to workaround this on our end. + Some implementations, like the `compression` npm package, do not implement the `response.write(data, callback)` signature, but `@whatwg-node/server` currently waits for the write callback to finish before ending the response stream. + This causes the response to hang when the compression middleware takes over for larger payloads. + This is a bug in the `compression` package (see [Express.js compression issue #46](https://github.com/expressjs/compression/issues/46)). + Since this mistake is common in other middleware, we prefer to work around it on our end.🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug incompressionpackage; [expressjs/compression#46](https://github.com/expr...(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. Now after calling `re...(NOUN_VERB_CONFUSION)
14-17: Refine workaround description for readability and accuracy.Lines 14–17 can be rephrased for better flow and correct punctuation. Suggested diff:
- Now after calling `response.write`, it no longer uses callback but first it checks the result; - if it is `true`, it means stream is drained and we can call `response.end` immediately. - else if it is `false`, it means the stream is not drained yet, so we can wait for the `drain` event to call `response.end`. + After calling `response.write`, we no longer rely on its callback; instead, we check its return value: + - If it returns `true`, the stream is drained, and we call `response.end` immediately. + - If it returns `false`, the stream is not yet drained, so we wait for the `drain` event before calling `response.end`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: You might be missing the article “a” here.
Context: ...ingresponse.write, it no longer uses callback but first it checks the result; if it ...(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ponse.write, it no longer uses callback but first it checks the result; if it is...(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ... the result; if it istrue, it means stream is drained and we can call `response.en...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~16-~16: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...it istrue, it means stream is drained and we can callresponse.endimmediately....(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...we can callresponse.endimmediately. else if it isfalse, it means the stream i...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.changeset/whole-planes-mate.md(1 hunks)packages/node-fetch/src/FormData.ts(1 hunks)packages/node-fetch/src/TransformStream.ts(2 hunks)packages/node-fetch/src/WritableStream.ts(3 hunks)packages/node-fetch/src/fetchNodeHttp.ts(4 hunks)packages/node-fetch/src/utils.ts(1 hunks)packages/promise-helpers/src/index.ts(1 hunks)packages/server/package.json(1 hunks)packages/server/src/createServerAdapter.ts(1 hunks)packages/server/src/utils.ts(7 hunks)packages/server/test/reproductions.spec.ts(2 hunks)packages/server/test/test-server.ts(1 hunks)test.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/node-fetch/src/TransformStream.ts
- test.mjs
- packages/node-fetch/src/fetchNodeHttp.ts
- packages/promise-helpers/src/index.ts
- packages/node-fetch/src/utils.ts
- packages/server/package.json
- packages/server/test/reproductions.spec.ts
- packages/server/src/createServerAdapter.ts
- packages/node-fetch/src/WritableStream.ts
- packages/server/test/test-server.ts
- packages/server/src/utils.ts
🧰 Additional context used
🪛 LanguageTool
.changeset/whole-planes-mate.md
[uncategorized] ~11-~11: The official spelling of this programming framework is “Express.js”.
Context: ...tually a bug in compression package; [expressjs/compression#46](https://github.com/expr...
(NODE_JS)
[grammar] ~12-~12: The word “workaround” is a noun. The verb is spelled with a white space.
Context: ...ce it is a common mistake, we prefer to workaround this on our end. Now after calling `re...
(NOUN_VERB_CONFUSION)
[uncategorized] ~14-~14: You might be missing the article “a” here.
Context: ...ing response.write, it no longer uses callback but first it checks the result; if it ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~14-~14: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ponse.write, it no longer uses callback but first it checks the result; if it is ...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ... the result; if it is true, it means stream is drained and we can call `response.en...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~16-~16: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...it is true, it means stream is drained and we can call response.end immediately....
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~16-~16: Possible missing comma found.
Context: ...we can call response.end immediately. else if it is false, it means the stream i...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e / azure-function
- GitHub Check: unit / node 22
- GitHub Check: unit / node 20
- GitHub Check: e2e / aws-lambda
- GitHub Check: unit / node 23
- GitHub Check: unit / node 18
- GitHub Check: unit / deno
- GitHub Check: unit / bun
🔇 Additional comments (4)
packages/node-fetch/src/FormData.ts (3)
97-100: Type correction forcurrentAsyncIteratorimproves type safetyThe type has been correctly updated to
AsyncIterator<Uint8Array>to match the actual data yielded by blob streams, fixing the type mismatch that was previously flagged in a review comment.
101-132: Implementation of incremental streaming with entry-by-entry processingThe new
handleNextEntryfunction implements a more efficient streaming approach that processes form data entries on demand rather than buffering them all upfront. The function properly handles boundaries between entries and correctly differentiates between string and Blob entries.
156-159: Good addition of cancellation handlingThe new cancellation handler properly cleans up both iterators when the stream is cancelled, which is essential for proper resource management in streaming operations.
.changeset/whole-planes-mate.md (1)
1-5: Metadata frontmatter is correct.The changeset header and package bump declarations are properly formatted and conform to the expected YAML frontmatter syntax.
Some implementations like
compressionnpm package do not implementresponse.write(data, callback)signature, but whatwg-node/server waits for it to finish the response stream.Then it causes the response stream hangs when the compression package takes the stream over when the response data is larger than its threshold.
It is actually a bug in
compressionpackage;expressjs/compression#46
But since it is a common mistake, we prefer to workaround this on our end.
Now after calling
response.write, it no longer uses callback but first it checks the result;if it is
true, it means stream is drained and we can callresponse.endimmediately.else if it is
false, it means the stream is not drained yet, so we can wait for thedrainevent to callresponse.end.This PR also does some improvements by unifying the "write and wait for drain logic".
And it removes the nodeRequestResponseMap stuff for performance gain.
Fixes graphql-hive/graphql-yoga#3997