Avoid stream/promises pipeline #2632
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis set of changes refactors the stream handling logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant fetchNodeHttp
participant nodeResponse
participant outputStream
Client->>fetchNodeHttp: Initiate fetch
fetchNodeHttp->>nodeResponse: Receive HTTP response
fetchNodeHttp->>outputStream: Create or use output stream
fetchNodeHttp->>outputStream: pipeThrough({ src: nodeResponse, dest: outputStream })
outputStream-->>fetchNodeHttp: Emits data or error events
fetchNodeHttp-->>Client: Returns response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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 (
|
ardatan
force-pushed
the
avoid-pipeline-promise
branch
from
f9e7821 to
a591646
Compare
✅
|
✅
|
✅
|
✅
|
✅
|
ardatan
force-pushed
the
avoid-pipeline-promise
branch
from
3302eb7 to
8ea06df
Compare
ardatan
force-pushed
the
avoid-pipeline-promise
branch
2 times, most recently
from
683ac7e to
27dea3e
Compare
ardatan
force-pushed
the
avoid-pipeline-promise
branch
from
f87c506 to
6c05e29
Compare
![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)
packages/node-fetch/src/utils.ts (1)
79-97: Consider potential race condition in abort handlingThe abort handler uses WeakRef which could theoretically be garbage collected between the check and the call. While unlikely in practice, consider storing a strong reference during the abort handler execution.
function onAbort() { - srcRef.deref()?.destroy(new AbortError()); + const src = srcRef.deref(); + if (src) { + src.destroy(new AbortError()); + } cleanup(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/chatty-cats-pull.md(1 hunks)packages/node-fetch/src/Body.ts(4 hunks)packages/node-fetch/src/fetchCurl.ts(2 hunks)packages/node-fetch/src/fetchNodeHttp.ts(3 hunks)packages/node-fetch/src/utils.ts(3 hunks)packages/server/test/reproductions.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:36.028Z
Learning: The node-fetch package tries different fetch implementations in the following order: 1) node-libcurl 2) undici 3) built-in undici in Node 4) node:http as fallback
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:47.290Z
Learning: The `handleNodeRequestAndResponse` method in `@whatwg-node/server` can be used to integrate with Fastify by passing both the request and reply objects in the route handler, along with the context containing `req` and `reply`.
.changeset/chatty-cats-pull.md (2)
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:36.028Z
Learning: The node-fetch package tries different fetch implementations in the following order: 1) node-libcurl 2) undici 3) built-in undici in Node 4) node:http as fallback
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:47.290Z
Learning: The handleNodeRequestAndResponse method in @whatwg-node/server can be used to integrate with Fastify by passing both the request and reply objects in the route handler, along with the context containing req and reply.
packages/node-fetch/src/fetchCurl.ts (2)
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:36.028Z
Learning: The node-fetch package tries different fetch implementations in the following order: 1) node-libcurl 2) undici 3) built-in undici in Node 4) node:http as fallback
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:47.290Z
Learning: The handleNodeRequestAndResponse method in @whatwg-node/server can be used to integrate with Fastify by passing both the request and reply objects in the route handler, along with the context containing req and reply.
packages/node-fetch/src/fetchNodeHttp.ts (2)
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:36.028Z
Learning: The node-fetch package tries different fetch implementations in the following order: 1) node-libcurl 2) undici 3) built-in undici in Node 4) node:http as fallback
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:47.290Z
Learning: The handleNodeRequestAndResponse method in @whatwg-node/server can be used to integrate with Fastify by passing both the request and reply objects in the route handler, along with the context containing req and reply.
packages/node-fetch/src/Body.ts (1)
Learnt from: ardatan
PR: ardatan/whatwg-node#0
File: :0-0
Timestamp: 2025-01-29T19:45:47.290Z
Learning: The handleNodeRequestAndResponse method in @whatwg-node/server can be used to integrate with Fastify by passing both the request and reply objects in the route handler, along with the context containing req and reply.
🧬 Code Graph Analysis (2)
packages/node-fetch/src/utils.ts (1)
packages/node-fetch/src/Request.ts (1)
signal(113-116)
packages/node-fetch/src/Body.ts (2)
packages/node-fetch/src/ReadableStream.ts (1)
PonyfillReadableStream(61-273)packages/promise-helpers/src/index.ts (1)
fakePromise(47-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: e2e / aws-lambda
- GitHub Check: e2e / azure-function
- GitHub Check: unit / bun
- GitHub Check: unit / node 18
- GitHub Check: unit / node 24
- GitHub Check: unit / node 20
- GitHub Check: unit / deno
- GitHub Check: alpha / snapshot
- GitHub Check: server (native)
- GitHub Check: server (undici)
- GitHub Check: server (ponyfill)
- GitHub Check: node-fetch (consumeBody)
- GitHub Check: node-fetch (noConsumeBody)
🔇 Additional comments (11)
packages/server/test/reproductions.spec.ts (1)
80-80: Why was the test assertion relaxed from exact content to just defined check?The test previously verified that
req.text()returns the exact body content 'hello world' when called outside the handler. Now it only checks if the result is defined. This reduces test coverage as it no longer validates that the body content is preserved correctly.Is this change intentional due to the new stream handling implementation, or should the exact content still be accessible?
.changeset/chatty-cats-pull.md (1)
1-5: LGTM!The changeset correctly documents this as a patch and clearly describes the change.
packages/node-fetch/src/fetchNodeHttp.ts (2)
45-49: Good simplification of signal assignmentThe removal of the redundant else branch makes the code cleaner while maintaining the same functionality.
117-132: Excellent error handling implementationThe use of
pipeThroughwith proper error handling that destroys both streams on error is a robust approach. The error callback ensures bothnodeResponseandoutputStreamare properly cleaned up.packages/node-fetch/src/utils.ts (2)
52-100: Well-implemented stream piping utilityThe
pipeThroughfunction is a robust replacement for the previous wrapper approach:
- Proper error propagation from source to destination
- Comprehensive abort signal handling with cleanup
- Good use of WeakRef to avoid memory leaks
- Clear comments explaining the behavior
114-120: Good implementation of AbortErrorThe AbortError class correctly follows Node.js conventions with appropriate name and message defaults.
packages/node-fetch/src/Body.ts (5)
10-10: Import changes look good.The addition of
fakePromiseandisArrayBufferViewimports is appropriate as both utilities are used throughout the file.
59-65: Good refactoring of signal handling.Separating signal assignment from body processing improves code organization and makes the responsibilities clearer.
73-73: Type consistency improvement.Using
undefinedinstead ofnullfor optional properties aligns with TypeScript best practices.
178-180: Good defensive check for destroyed streams.This prevents potential errors when attempting to read from destroyed streams and maintains consistency by returning an empty array.
455-461: Function signature simplified appropriately.Removing the unused
signalparameter fromprocessBodyInitaligns with the single responsibility principle and matches the refactored constructor logic.
Inherits and closes #2638
Fixes the overhead of
pipelinefunction so it catches the performance ofFeb 2025version.Older version performance ardatan/feTS#2915
Regression ardatan/feTS#2918
pipelineonly forfetchfrom Avoid Node's promise pipeline by piping streams directly #2633~9% increase in perf benchmarking Hive Gateway, memory consumption feels lower too
~20% increase in perf benchmarking feTS