-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Types: replacing ReadableStream and WritableStream #18666
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
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughVersion in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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.
Thanks for testing the TS build. I just made a formatting suggestion to fix a lint error. Otherwise it looks good to me.
Co-authored-by: js07 <19861096+js07@users.noreply.github.com>
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.
LGTM!
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
types/src/index.ts (2)
1-3
: Consider using "node:stream" import and addressing formatting suggestion.The import statement is functional, but you could improve it by:
- Using the
node:
prefix for built-in modules (modern Node.js best practice)- Addressing the previous review comment about formatting
Apply this diff:
-import { - Readable, Writable, -} from "stream"; +import { + Readable, + Writable, +} from "node:stream";
127-127
: Consider method naming consistency (optional).The parameter type change from
ReadableStream
toReadable
is correct. Optionally, you might consider renaming the method fromfromReadableStream
tofromReadable
or similar to avoid confusion, since it now accepts Node.jsReadable
streams rather than Web APIReadableStream
. However, this would be a breaking change and may not be worth the disruption.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
types/src/index.ts
(3 hunks)
⏰ 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). (2)
- GitHub Check: Lint Code Base
- GitHub Check: pnpm publish
🔇 Additional comments (2)
types/src/index.ts (2)
121-122
: LGTM!The migration from Web Streams to Node.js streams for
createReadStream()
andcreateWriteStream()
is correct and consistent with the overall changes in this PR.
76-76
: Document and verify Node.js Readable streams breaking change. HTTPResponse.body now uses Node.js Readable instead of Web Streams—ensure any runtime handlers or examples consuming this property are updated accordingly and note this breaking change in the CHANGELOG.
Follow-up to #18511
Note: I pointed all packages that import types to a symlink to build them with these changes applied. The build was successful.
Summary by CodeRabbit