Conversation
Enable urllib to run on Bun by working around differences in Bun's built-in undici implementation: - Use AbortSignal.timeout() for request timeouts (Bun ignores headersTimeout/bodyTimeout), compose with user signal via AbortSignal.any() - Convert Node.js Readable streams to Buffer/Web ReadableStream for request bodies (Bun's undici can't consume Node.js streams) - Skip manual decompression (Bun auto-decompresses gzip/br) - Handle Bun-specific error types (TimeoutError, ConnectionClosed) - Wrap non-extensible error objects via Object.isExtensible() check - Adapt User-Agent header to show Bun version - Add FormData.toBuffer() for Bun compatibility - Guard fetch internals against Bun's different Response state - Skip tests for unsupported Bun features (H2, diagnostics_channel, unix socket, MockAgent, rejectUnauthorized) Node.js: 40 test files passed, 202 tests passed Bun: 37 test files passed, 143 tests passed, 0 failures Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds comprehensive Bun runtime support to the urllib HTTP client. It introduces runtime detection, modifies request and response handling for Bun's undici-based environment (including form-data buffering and timeout management), updates the user-agent header, adds error mapping for Bun-specific errors, and conditionally skips incompatible tests. A new CI workflow tests the library against Bun on multiple platforms. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements compatibility for the Bun runtime by addressing limitations in Bun's undici implementation. Key changes include adding a toBuffer method to FormData, implementing a fallback timeout mechanism using AbortSignal.timeout, and adapting file and stream handling to use memory buffers or Web Streams. The test suite was also updated to skip Node.js-specific features when running in Bun. Review feedback identifies potential memory risks associated with buffering large data in Bun and suggests adjusting the reported timeout values in error messages to accurately reflect the combined headers and body timeout duration.
| async toBuffer(): Promise<Buffer> { | ||
| return new Promise<Buffer>((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| this.on('data', (chunk: Buffer | string) => { | ||
| // CombinedStream emits boundary/header strings alongside Buffer data | ||
| chunks.push(typeof chunk === 'string' ? Buffer.from(chunk) : chunk); | ||
| }); | ||
| this.on('end', () => resolve(Buffer.concat(chunks))); | ||
| this.on('error', reject); | ||
| // CombinedStream pauses by default and only starts | ||
| // flowing when piped or explicitly resumed | ||
| this.resume(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This method buffers the entire FormData stream into memory. While the comment explains this is for Bun compatibility, it's important to be aware of the memory implications. For large forms with large files, this could lead to high memory usage and potential performance problems. This is a significant trade-off for compatibility.
| if (isBun) { | ||
| // Bun's CombinedStream can't pipe Node.js streams | ||
| const streamChunks: Buffer[] = []; | ||
| for await (const chunk of file) { | ||
| streamChunks.push(Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); | ||
| } | ||
| value = Buffer.concat(streamChunks); |
There was a problem hiding this comment.
This implementation buffers the entire stream into memory before sending the request when running on Bun. This introduces a risk of high memory consumption for large file uploads, which could lead to performance issues or out-of-memory errors in memory-constrained environments.
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); | ||
| } else if (isBun && err.name === 'TypeError' && /timed?\s*out|timeout/i.test(err.message)) { | ||
| // Bun may wrap timeout as TypeError | ||
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); |
There was a problem hiding this comment.
The timeout value passed to HttpClientRequestTimeoutError is headersTimeout || bodyTimeout, which can be misleading. For Bun, you are creating a single timeout of headersTimeout + bodyTimeout. The error message should reflect this total timeout duration to avoid confusion. For example, if headersTimeout is 400 and bodyTimeout is 500, the actual timeout is 900ms, but the error will state 400ms.
I suggest using headersTimeout + bodyTimeout here to be consistent.
Note that this will require updating the corresponding test in test/options.timeout.test.ts to expect the correct total timeout value.
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); | |
| } else if (isBun && err.name === 'TypeError' && /timed?\s*out|timeout/i.test(err.message)) { | |
| // Bun may wrap timeout as TypeError | |
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); | |
| err = new HttpClientRequestTimeoutError(headersTimeout + bodyTimeout, { cause: err }); | |
| } else if (isBun && err.name === 'TypeError' && /timed?\s*out|timeout/i.test(err.message)) { | |
| // Bun may wrap timeout as TypeError | |
| err = new HttpClientRequestTimeoutError(headersTimeout + bodyTimeout, { cause: err }); |
Run tests on Bun runtime across ubuntu and macos. Patches vitest imports from vite-plus to vanilla vitest since Bun can't use the vite-plus wrapper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds Bun runtime compatibility to urllib by adapting request/response handling to Bun’s built-in undici differences, while conditionally skipping/loosening tests for features Bun doesn’t support yet.
Changes:
- Add Bun runtime detection (
isBun) and adjust default User-Agent to include Bun version. - Implement Bun-specific request handling: signal-based timeout fallback, request-body conversions for FormData/streams, and skip manual decompression.
- Update test suite to skip or relax assertions for Bun-incompatible undici features (H2, unix sockets, diagnostics_channel, MockAgent, etc.).
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/HttpClient.ts | Core Bun runtime adaptations (UA, timeout fallback signal, body conversions, decompression behavior, error wrapping). |
| src/FormData.ts | Adds FormData.toBuffer() to serialize multipart bodies for Bun compatibility. |
| src/fetch.ts | Makes fetch diagnostics more tolerant when Bun’s Response doesn’t match undici internal state. |
| test/user-agent.test.ts | Accept Bun User-Agent; skip UA-empty/undefined tests on Bun. |
| test/urllib.options.rejectUnauthorized-false.test.ts | Skip Bun where undici Agent lacks rejectUnauthorized connect option and H2. |
| test/options.writeStream.test.ts | Skip Bun cases impacted by compression + timeout/writeStream behaviors. |
| test/options.timing.test.ts | Skip Bun timing assertions (timing not populated). |
| test/options.timeout.test.ts | Adjust timeout assertions to tolerate Bun error types/wrapping; skip H2 timeout test on Bun. |
| test/options.streaming.test.ts | Skip Bun compressed streaming test; skip large (1GB) streaming test due to Bun slowness/timeouts. |
| test/options.stream.test.ts | Skip Bun stream-timeout and stream-error behaviors. |
| test/options.socketPath.test.ts | Skip Bun (unix socket support not available/compatible). |
| test/options.socketErrorRetry.test.ts | Relax socket error retry expectations; skip Bun retry behavior tests. |
| test/options.signal.test.ts | Relax abort error message match; skip Bun EventEmitter-based abort. |
| test/options.reset.test.ts | Skip reset/connection-close assertion on Bun. |
| test/options.opaque.test.ts | Skip Bun opaque-on-error test. |
| test/options.headers.test.ts | Skip Bun header normalization/default-header behavior assertions. |
| test/options.gzip.test.ts | Skip Bun gzip/br behavior tests (auto-decompression differences). |
| test/options.followRedirect.test.ts | Skip Bun “disable auto redirect” behavior test. |
| test/options.fixJSONCtlChars.test.ts | Skip Bun invalid JSON control-char test. |
| test/options.dataType.test.ts | Skip Bun invalid JSON format assertion test. |
| test/options.data.test.ts | Skip Bun cases involving preserving Readable request bodies. |
| test/options.content.test.ts | Skip Bun POST with Readable content body. |
| test/options.compressed.test.ts | Skip Bun compressed default/invalid compressed content tests. |
| test/non-ascii-request-header.test.ts | Accept Bun’s TypeError for invalid header instead of undici InvalidArgumentError. |
| test/keep-alive-header.test.ts | Skip keep-alive header assertions on Bun (headers stripped internally). |
| test/index.test.ts | Skip Bun set-cookie string and socket-end error tests; skip mocking suite on Bun. |
| test/head-request-should-keepalive.test.ts | Skip Bun connection header + socket reuse assertions and reset expectations. |
| test/formData-with-BufferStream.test.ts | Adds isBun import (Bun-specific behavior coverage setup). |
| test/fetch.test.ts | Skip Bun fetch diagnostics tests. |
| test/diagnostics_channel.test.ts | Skip entire suite on Bun (diagnostics_channel limitations). |
| test/HttpClient.test.ts | Skip Bun suites for allowH2/lookup/checkAddress. |
| test/HttpClient.events.test.ts | Adjust expectations for timing/socket/error fields under Bun. |
| test/HttpClient.connect.rejectUnauthorized.test.ts | Skip Bun rejectUnauthorized connect option test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| headers['content-type'] = 'text/plain;charset=UTF-8'; | ||
| } | ||
| isStreamingRequest = isReadable(args.content); | ||
| isStreamingRequest = !isBun && isReadable(args.content); |
There was a problem hiding this comment.
On Bun, args.content can still be a Node.js Readable (you later convert requestOptions.body via Readable.toWeb). However isStreamingRequest is currently forced to false on Bun (!isBun && isReadable(args.content)), which means retries/socketErrorRetry/redirects may stay enabled for a non-rewindable streaming body. That can break retries/redirect handling (subsequent attempts will re-use an already-consumed stream). Consider marking isStreamingRequest whenever the body is stream-like regardless of runtime (and disabling retry/redirect accordingly), even if Bun requires a Web stream conversion.
| isStreamingRequest = !isBun && isReadable(args.content); | |
| isStreamingRequest = isReadable(args.content); |
| if (isBun) { | ||
| // Bun's CombinedStream can't pipe Node.js streams | ||
| const streamChunks: Buffer[] = []; | ||
| for await (const chunk of file) { |
There was a problem hiding this comment.
In the Bun path for args.files, you buffer non-Buffer inputs by iterating with for await (const chunk of file). This will throw if file is only “readable-like” (passes isReadable) but is not actually async-iterable. To avoid runtime errors for legacy/old-style streams, wrap non-Readable streams into a real Readable (e.g., via new Readable().wrap(...)) or check for Symbol.asyncIterator before using for await.
| for await (const chunk of file) { | |
| const sourceStream: any = | |
| typeof (file as any)[Symbol.asyncIterator] === 'function' | |
| ? file | |
| : new Readable({ read() {} }).wrap(file as any); | |
| for await (const chunk of sourceStream) { |
| this.on('data', (chunk: Buffer | string) => { | ||
| // CombinedStream emits boundary/header strings alongside Buffer data | ||
| chunks.push(typeof chunk === 'string' ? Buffer.from(chunk) : chunk); | ||
| }); | ||
| this.on('end', () => resolve(Buffer.concat(chunks))); | ||
| this.on('error', reject); |
There was a problem hiding this comment.
FormData.toBuffer() attaches data/end/error listeners with .on(...) but never removes them. If the same FormData instance is re-used or toBuffer() is called more than once, listeners can accumulate and cause leaks or duplicate resolution attempts. Prefer once(...) handlers and/or explicitly remove listeners in both resolve and reject paths.
| this.on('data', (chunk: Buffer | string) => { | |
| // CombinedStream emits boundary/header strings alongside Buffer data | |
| chunks.push(typeof chunk === 'string' ? Buffer.from(chunk) : chunk); | |
| }); | |
| this.on('end', () => resolve(Buffer.concat(chunks))); | |
| this.on('error', reject); | |
| this.once('data', (chunk: Buffer | string) => { | |
| // CombinedStream emits boundary/header strings alongside Buffer data | |
| chunks.push(typeof chunk === 'string' ? Buffer.from(chunk) : chunk); | |
| }); | |
| this.once('end', () => resolve(Buffer.concat(chunks))); | |
| this.once('error', reject); |
| } catch { | ||
| state = {}; |
There was a problem hiding this comment.
This try/catch around getResponseState(res) is unconditional, so any unexpected failure in Node (e.g., undici internal API changes) will now be silently swallowed and timingInfo will be dropped without any signal. To keep Node behavior fail-fast while still supporting Bun, consider gating the fallback to Bun only (or catching only the expected error type and logging/debugging when it happens).
| } catch { | |
| state = {}; | |
| } catch (err) { | |
| // Only fall back silently on Bun, where getResponseState is expected to fail. | |
| const isBun = typeof (globalThis as any).Bun !== 'undefined'; | |
| if (isBun) { | |
| state = {}; | |
| } else { | |
| debug('getResponseState failed unexpectedly: %o', err); | |
| throw err; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (13)
test/options.opaque.test.ts (1)
35-53: Avoid fully skipping opaque-on-error coverage for Bun.Even if Bun emits different error names/messages, this test should still assert that
err.opaqueis preserved. Consider runtime-conditional assertions instead of skipping the case entirely.Suggested adjustment
- it.skipIf(isBun)('should opaque work on error request', async () => { + it('should opaque work on error request', async () => { await assert.rejects( async () => { await urllib.request(`${_url}socket.end.error`, { opaque: { traceId: 'some random id here', }, }); }, (err: any) => { - assert.equal(err.res.status, 200); - assert.equal(err.name, 'HTTPParserError'); + if (!isBun) { + assert.equal(err.res.status, 200); + assert.equal(err.name, 'HTTPParserError'); + } assert.deepEqual(err.opaque, { traceId: 'some random id here', }); return true; }, ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.opaque.test.ts` around lines 35 - 53, Remove the full skip for Bun and ensure the test always verifies that the opaque object is preserved on error: change the skipped test (the it.skipIf(isBun) block around the 'should opaque work on error request' case) to run unconditionally and inside the assert.rejects callback keep the check assert.deepEqual(err.opaque, { traceId: 'some random id here' }) but make the assertion on err.name/runtime-specific error conditional (e.g. if (isBun) { assert.ok(['SomeBunErrorName', 'HTTPParserError'].includes(err.name)) } else { assert.equal(err.name, 'HTTPParserError') }), keeping the other checks on err.res.status and using the same urllib.request call with opaque to locate the code.test/options.timing.test.ts (1)
24-84: Add a minimal Bun timing contract test if detailed timings are runtime-specific.If exact phase timings are unstable on Bun, keep at least a lightweight assertion path (timing object exists, fields are non-negative,
rt > 0) instead of skipping both positive-timing cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.timing.test.ts` around lines 24 - 84, The two skipped tests ('should timing = true work' and 'should timing default to true' in test/options.timing.test.ts) lose Bun coverage; update them so when isBun is true they perform a minimal Bun-friendly contract: request with timing: true (or default) and assert response.status === 200, res is RawResponseWithMeta, res.timing exists, each timing field (e.g., waiting, queuing, connected, requestHeadersSent, requestSent, contentDownload) is >= 0 (not > 0), and res.rt > 0; keep the existing stricter assertions for non-Bun runtimes (isBun false) inside the same test by branching on isBun so you don't fully skip the tests.test/options.content.test.ts (1)
166-184: Keep a Bun assertion path for readablecontentuploads.Since Bun stream-body adaptation is part of this PR, fully skipping this case drops important coverage. Keep the test and relax Bun-specific header assertions if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.content.test.ts` around lines 166 - 184, The test "should POST content = readable" was fully skipped for Bun but we need to keep it and relax Bun-specific header assertions: remove the overall skipIf(isBun) and instead run the test for all runtimes, and inside the test guard the header assertions using the isBun flag (or equivalent) so that when isBun is true you skip or adjust checks for 'transfer-encoding' and 'connection' headers while keeping all other assertions (status, data length/content equality, x-request-headers parsing, URL/redirect checks) intact; locate the test block (the it.skipIf/isBun wrapper), the urllib.request call, and the assertions for requestHeaders['transfer-encoding'] and requestHeaders.connection to apply the conditional checks.test/options.writeStream.test.ts (1)
52-159: Consider Bun-specific assertions instead of skipping all writeStream edge cases.These tests protect important invariants (
writeStreamclose/end behavior on timeout/error). Prefer keeping them active with Bun-adjusted expectations rather than blanket skips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.writeStream.test.ts` around lines 52 - 159, Tests currently skip writeStream edge cases on Bun; instead remove skipIf(isBun) and make Bun-specific assertions so behavior is validated across runtimes. For each test named 'should work with compressed=true/false', 'should close writeStream when request timeout', 'should throw request error when writeStream error', and 'should end writeStream when server error' replace the top-level skip with runtime branches using the isBun flag: keep the same setup (createWriteStream, urllib.request) but assert alternative expectations when isBun is true (e.g. different destroyed/close/error semantics) and keep the original assertions for non-Bun. Reference symbols: createWriteStream, urllib.request, writeStream.destroyed, writeStream.on('close'), writeStream.on('error'), writeStreamClosed, writeStreamError, and the existing process.version v16 check; implement concise if (isBun) { /* Bun expectations */ } else { /* original asserts */ } blocks in each test.test/options.gzip.test.ts (2)
100-100: Minor typo: "event" should be "even" in test description.✏️ Fix typo
- it.skipIf(isBun)('should use compressed = false event gzip = true', async () => { + it.skipIf(isBun)('should use compressed = false even when gzip = true', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.gzip.test.ts` at line 100, The test description string for the skipped test uses a typo: "event" should be "even"; update the test title in the it.skipIf(isBun) call (the string 'should use compressed = false event gzip = true') to read 'should use compressed = false even gzip = true' so the human-readable test name is correct (locate the it.skipIf(isBun) test declaration in test/options.gzip.test.ts and change the quoted description).
35-35: Add comments explaining why these compression tests are skipped on Bun.Based on
src/HttpClient.ts:704-718, Bun's undici auto-decompresses response bodies transparently. Tests withgzip=falseorcompressed=falseverify manual decompression control, which is inapplicable when Bun handles decompression automatically.💡 Suggested comments
+ // Bun: auto-decompresses responses, can't test disabled decompression it.skipIf(isBun)('should handle gzip text response on gzip = false', async () => {+ // Bun: auto-decompresses responses, can't test disabled decompression it.skipIf(isBun)('should handle gzip json response on gzip = false', async () => {+ // Bun: auto-decompresses responses, can't test disabled decompression it.skipIf(isBun)('should handle br json response on gzip = false', async () => {+ // Bun: auto-decompresses responses, compressed=false has no effect it.skipIf(isBun)('should use compressed = false event gzip = true', async () => {Also applies to: 61-61, 87-87, 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.gzip.test.ts` at line 35, Add a short comment above the skipped tests (those using isBun and patterns like "should handle gzip text response on gzip = false" and other skipped cases) explaining that Bun's undici auto-decompresses response bodies so tests that assert behavior when gzip=false or compressed=false (manual decompression control) are not applicable; reference the HttpClient behavior in src/HttpClient.ts around the undici auto-decompression logic (lines ~704-718) so future readers know why these tests are skipped on Bun.test/options.data.test.ts (1)
505-505: Add comments explaining why these readable stream tests are skipped on Bun.Per the PR description, Bun's undici does not consume Node.js
Readablestreams directly. These tests useReadable.from()which would fail on Bun without the stream-to-buffer conversion thatHttpClient.tsimplements for request bodies.💡 Suggested comments
+ // Bun: Node.js Readable streams as request body require conversion it.skipIf(isBun)('should keep data to readable when content-type exists', async () => {+ // Bun: Node.js Readable streams as request body require conversion it.skipIf(isBun)('should keep data to readable and not set content-type', async () => {Also applies to: 531-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.data.test.ts` at line 505, The skipped tests "should keep data to readable when content-type exists" and the similar one at the other location are missing explanatory comments; update the test file test/options.data.test.ts by adding brief comments above the it.skipIf(isBun) blocks that state Bun's undici does not consume Node.js Readable streams (e.g. Readable.from()), so these tests rely on HttpClient.ts's stream-to-buffer conversion and will fail on Bun without it—mention Readable.from() and HttpClient.ts behavior so future readers understand why the skip is necessary.test/options.streaming.test.ts (1)
58-58: Add a comment explaining why this test is skipped on Bun.Line 141 has a helpful comment (
// Bun: 1GB streaming transfer too slow...), but line 58 lacks explanation. Based onsrc/HttpClient.ts:704-718, Bun auto-decompresses response bodies, making thecompressed=true/falsebehavior assertions invalid on Bun.💡 Suggested comment
- it.skipIf(isBun)('should work on streaming=true and compressed=true/false', async () => { + // Bun: auto-decompresses responses, so manual decompression assertions don't apply + it.skipIf(isBun)('should work on streaming=true and compressed=true/false', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.streaming.test.ts` at line 58, Add an inline comment next to the skipped test declaration (the it.skipIf(isBun) for 'should work on streaming=true and compressed=true/false') explaining that the test is skipped on Bun because Bun auto-decompresses response bodies (see HttpClient behavior around src/HttpClient.ts:704-718), which invalidates assertions about compressed=true/false; mention that this causes streaming/compressed behavior differences on Bun and reference isBun as the guard.test/HttpClient.test.ts (1)
57-57: Consider adding brief comments for skipped describe blocks.While the PR description documents that HTTP/2, custom DNS lookup, and
checkAddressaren't supported on Bun, inline comments would help future maintainers quickly understand why these entire test suites are skipped.💡 Suggested comments
+ // Bun's undici doesn't support HTTP/2 describe.skipIf(isBun)('clientOptions.allowH2', () => {+ // Bun's undici doesn't support custom DNS lookup describe.skipIf(isBun)('clientOptions.lookup', () => {+ // Bun's undici doesn't support checkAddress for SSRF protection describe.skipIf(isBun)('clientOptions.checkAddress', () => {Also applies to: 259-259, 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/HttpClient.test.ts` at line 57, Add a brief inline comment above each describe.skipIf(isBun) block explaining why the suite is skipped on Bun (e.g., "Bun does not support HTTP/2, custom DNS lookup, or socket checkAddress, so these tests are skipped on Bun"). Specifically update the describe.skipIf(isBun)('clientOptions.allowH2') block and the other skipped describe blocks for custom DNS lookup and checkAddress to include that concise rationale so future maintainers understand the reason at a glance.test/options.stream.test.ts (1)
107-107: Add skip comments explaining why these tests are skipped on Bun.Other skipped tests in this PR include brief comments (e.g., lines 27-28 in
urllib.options.rejectUnauthorized-false.test.ts). These three skipped tests lack explanatory comments, making it harder for future maintainers to understand the Bun-specific limitations.💡 Suggested comments
- it.skipIf(isBun)('should close 1KB request stream when request timeout', async () => { + // Bun: timeout handling differs for stream requests + it.skipIf(isBun)('should close 1KB request stream when request timeout', async () => {- it.skipIf(isBun)('should close 10MB size request stream when request timeout', async () => { + // Bun: timeout handling differs for stream requests + it.skipIf(isBun)('should close 10MB size request stream when request timeout', async () => {- it.skipIf(isBun)('should throw request error when stream error', async () => { + // Bun: stream error propagation differs from Node.js + it.skipIf(isBun)('should throw request error when stream error', async () => {Also applies to: 132-132, 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.stream.test.ts` at line 107, Add brief explanatory comments above each it.skipIf(isBun) test in test/options.stream.test.ts (the tests with descriptions "should close 1KB request stream when request timeout" and the other two at the same pattern) that explain why they are skipped on Bun (e.g., missing stream timeout behavior, platform-specific socket handling, or known Bun bug/limitation), mirroring the style used in urllib.options.rejectUnauthorized-false.test.ts; place the comment immediately above the it.skipIf call so future maintainers can quickly see the Bun-specific reason and any issue/PR reference if available.test/index.test.ts (1)
57-57: Add comments explaining why these tests/suites are skipped on Bun.The PR description mentions MockAgent is unsupported, but the other two skips (
set-cookiestring handling andsocket.enderror handling) lack documentation explaining the Bun-specific differences.💡 Suggested comments
+ // Bun: set-cookie header handling differs from Node.js undici it.skipIf(isBun)('should response set-cookie as a string', async () => {+ // Bun: HTTP parser error handling differs from Node.js it.skipIf(isBun)('should handle server socket end("balabal") will error', async () => {+ // Bun: MockAgent is not supported describe.skipIf(isBun)('Mocking request', () => {Also applies to: 149-149, 191-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` at line 57, Three tests are being skipped on Bun without explanations; add brief inline comments above each skipped test (the it.skipIf(isBun) blocks for "should response set-cookie as a string", the other set-cookie test, and the socket.end error handling test) explaining the Bun-specific reason (e.g., MockAgent unsupported on Bun and any behavior differences in set-cookie string handling or socket.end error semantics) so reviewers know why these are skipped; reference the test names ("should response set-cookie as a string", the other set-cookie test, and the socket.end error handling test) and update the test file to include a one-line comment per skipped test describing the exact Bun limitation or behavioral divergence.test/options.reset.test.ts (1)
29-31: Add a Bun-side assertion to avoid a no-op branch.At Line [29], Bun currently skips the connection check entirely. Consider asserting that the header is not
'close'(or is absent) so this branch still validates expected Bun behavior.Proposed test tightening
- if (!isBun) { - assert(response.data.headers.connection === 'close'); - } + if (isBun) { + assert.notEqual(response.data.headers.connection, 'close'); + } else { + assert.equal(response.data.headers.connection, 'close'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.reset.test.ts` around lines 29 - 31, The test currently only asserts connection === 'close' inside the if (!isBun) branch, leaving Bun as a no-op; update the test in options.reset.test.ts to add an else branch for when isBun is true that asserts the response.data.headers.connection is not 'close' (or is undefined/null) so the Bun behavior is validated; locate the isBun check and response.data.headers.connection usage and add the complementary assertion in the else branch to tighten the test.test/HttpClient.events.test.ts (1)
113-119: Bun error assertions are currently too permissive.Line [115] and Line [137] only check truthiness of
name, which won’t catch many regressions. Prefer stable-but-strong invariants (name/messageare strings, and error metadata is present).Proposed assertion strengthening
if (isBun) { // Bun throws different error types for socket errors - assert(info.error.name); + assert.equal(typeof info.error.name, 'string'); + assert.equal(typeof info.error.message, 'string'); } else { assert.equal(info.error.name, 'SocketError'); assert.equal(info.error.message, 'other side closed'); } @@ assert(err.status === -1 || err.status === undefined); if (isBun) { - assert(err.name); + assert.equal(typeof err.name, 'string'); + assert.equal(typeof err.message, 'string'); + assert(err.res); } else { assert(err.res); assert.equal(err.name, 'SocketError'); assert.equal(err.message, 'other side closed'); }Also applies to: 136-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/HttpClient.events.test.ts` around lines 113 - 119, The Bun-specific assertions in the test rely only on truthiness (isBun branch) and are too weak; update the checks around the info object (variable info) and the isBun branch to assert stable invariants instead of just presence: verify that info.error exists, that typeof info.error.name === 'string' and typeof info.error.message === 'string', and that any expected metadata fields (e.g., code or errno if present) are present or of the correct type; apply the same strengthened assertions to the other Bun branch later in the file so both occurrences validate string types for name/message and required metadata rather than mere truthiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/HttpClient.ts`:
- Around line 117-126: The User-Agent construction currently swaps the canonical
"Node.js/..." token for "Bun/..." when running in Bun; change getRuntimeInfo so
it always returns the canonical Node.js token (e.g.
`Node.js/${process.version.substring(1)}`) and adjust HEADER_USER_AGENT to
append a separate Bun token only when running under Bun (e.g. add `
Bun/${process.versions.bun}` when isBun is true) so HEADER_USER_AGENT preserves
`node-urllib/${VERSION} Node.js/{version} (...)` and surfaces Bun separately;
update references in getRuntimeInfo and HEADER_USER_AGENT (and use VERSION)
accordingly.
- Around line 557-563: When running in Bun and args.content is a FormData, the
code currently buffers it to a Buffer but forgets to copy multipart headers
(boundary) from FormData.getHeaders(), so the content-type/boundary is lost;
update the isBun branch where args.content is handled (look for isBun,
args.content, requestOptions.body and FormData casting) to call (args.content as
FormData).getHeaders() and merge its content-type into the headers map
before/after setting requestOptions.body so the multipart boundary is preserved
(ensure headers['content-type'] is set from getHeaders() when present).
- Around line 816-821: The Bun timeout branches (checks on isBun and err.name)
currently construct HttpClientRequestTimeoutError using headersTimeout ||
bodyTimeout, which hides the actual timeout when the code uses the fallback
abort signal that fires after headersTimeout + bodyTimeout; update those
branches to compute and pass the real enforced timeout (e.g., use headersTimeout
+ bodyTimeout when both are set, otherwise use the one that exists) into
HttpClientRequestTimeoutError so the error reports the correct timeout value
(refer to isBun, the err.name checks, HttpClientRequestTimeoutError,
headersTimeout, and bodyTimeout to locate the changes).
- Line 567: The code treats Bun stream bodies as replayable by setting
isStreamingRequest = !isBun && isReadable(args.content); instead, detect
readable streams regardless of isBun and ensure you disable redirect and
socketErrorRetry for those cases: update the isStreamingRequest logic to
consider isReadable(args.content) even when isBun, and in the request-paths (the
blocks referencing redirect and socketErrorRetry around where
options.content/args.content are used) explicitly turn off redirect and
socketErrorRetry when args.content or options.content is a readable stream so
streaming requests are not retried or redirected.
- Around line 521-523: The code currently forces Bun multipart streams into
memory via readFile / Buffer.concat / toBuffer which can OOM for large uploads;
update the HttpClient upload handling (check isBun and the multipart file/stream
branches in the upload/multipart logic) to avoid silently materializing streams:
detect when a file input is a Readable/stream and on Bun either (a) throw a
clear, documented error rejecting streaming uploads on Bun with guidance to
provide a file path or a Buffer, or (b) enforce a configurable size cap (e.g.,
maxUploadBytes) and reject if the stream exceeds it; do not call readFile or
Buffer.concat unconditionally for stream inputs—either require file paths (use
createReadStream) or write to a temporary file on disk with streaming I/O before
building the multipart body so backpressure is preserved; ensure the change
touches the isBun/readFile/createReadStream/toBuffer/Buffer.concat code paths
used for multipart handling.
- Around line 443-448: The build fails because AbortSignal.any() only accepts
AbortSignal, but args.signal can be an EventEmitter; update the Bun-specific
block that creates requestSignal (around isBun, AbortSignal.timeout,
headersTimeout, bodyTimeout) to first narrow args.signal to an AbortSignal
(e.g., check typeof/instance or a type guard) and only call
AbortSignal.any([bunTimeoutSignal, args.signal]) when args.signal is an
AbortSignal; otherwise handle the EventEmitter case separately (for example, use
bunTimeoutSignal alone or convert/subscribe the EventEmitter without passing it
into AbortSignal.any).
In `@test/options.socketErrorRetry.test.ts`:
- Around line 45-50: The Bun branch in the test uses a non-strict assertion
(assert(err.res.socketErrorRetries >= 0)) which won't catch regressions; tighten
it to validate retry behavior by asserting a meaningful minimum or exact value
for err.res.socketErrorRetries when isBun is true (e.g.,
assert(err.res.socketErrorRetries >= 1) or
assert.equal(err.res.socketErrorRetries, expectedBunRetries)), updating the test
in options.socketErrorRetry.test.ts to use that stricter check alongside the
existing non-Bun assert.equal path so the socket retry behavior for the isBun
branch is actually verified.
---
Nitpick comments:
In `@test/HttpClient.events.test.ts`:
- Around line 113-119: The Bun-specific assertions in the test rely only on
truthiness (isBun branch) and are too weak; update the checks around the info
object (variable info) and the isBun branch to assert stable invariants instead
of just presence: verify that info.error exists, that typeof info.error.name ===
'string' and typeof info.error.message === 'string', and that any expected
metadata fields (e.g., code or errno if present) are present or of the correct
type; apply the same strengthened assertions to the other Bun branch later in
the file so both occurrences validate string types for name/message and required
metadata rather than mere truthiness.
In `@test/HttpClient.test.ts`:
- Line 57: Add a brief inline comment above each describe.skipIf(isBun) block
explaining why the suite is skipped on Bun (e.g., "Bun does not support HTTP/2,
custom DNS lookup, or socket checkAddress, so these tests are skipped on Bun").
Specifically update the describe.skipIf(isBun)('clientOptions.allowH2') block
and the other skipped describe blocks for custom DNS lookup and checkAddress to
include that concise rationale so future maintainers understand the reason at a
glance.
In `@test/index.test.ts`:
- Line 57: Three tests are being skipped on Bun without explanations; add brief
inline comments above each skipped test (the it.skipIf(isBun) blocks for "should
response set-cookie as a string", the other set-cookie test, and the socket.end
error handling test) explaining the Bun-specific reason (e.g., MockAgent
unsupported on Bun and any behavior differences in set-cookie string handling or
socket.end error semantics) so reviewers know why these are skipped; reference
the test names ("should response set-cookie as a string", the other set-cookie
test, and the socket.end error handling test) and update the test file to
include a one-line comment per skipped test describing the exact Bun limitation
or behavioral divergence.
In `@test/options.content.test.ts`:
- Around line 166-184: The test "should POST content = readable" was fully
skipped for Bun but we need to keep it and relax Bun-specific header assertions:
remove the overall skipIf(isBun) and instead run the test for all runtimes, and
inside the test guard the header assertions using the isBun flag (or equivalent)
so that when isBun is true you skip or adjust checks for 'transfer-encoding' and
'connection' headers while keeping all other assertions (status, data
length/content equality, x-request-headers parsing, URL/redirect checks) intact;
locate the test block (the it.skipIf/isBun wrapper), the urllib.request call,
and the assertions for requestHeaders['transfer-encoding'] and
requestHeaders.connection to apply the conditional checks.
In `@test/options.data.test.ts`:
- Line 505: The skipped tests "should keep data to readable when content-type
exists" and the similar one at the other location are missing explanatory
comments; update the test file test/options.data.test.ts by adding brief
comments above the it.skipIf(isBun) blocks that state Bun's undici does not
consume Node.js Readable streams (e.g. Readable.from()), so these tests rely on
HttpClient.ts's stream-to-buffer conversion and will fail on Bun without
it—mention Readable.from() and HttpClient.ts behavior so future readers
understand why the skip is necessary.
In `@test/options.gzip.test.ts`:
- Line 100: The test description string for the skipped test uses a typo:
"event" should be "even"; update the test title in the it.skipIf(isBun) call
(the string 'should use compressed = false event gzip = true') to read 'should
use compressed = false even gzip = true' so the human-readable test name is
correct (locate the it.skipIf(isBun) test declaration in
test/options.gzip.test.ts and change the quoted description).
- Line 35: Add a short comment above the skipped tests (those using isBun and
patterns like "should handle gzip text response on gzip = false" and other
skipped cases) explaining that Bun's undici auto-decompresses response bodies so
tests that assert behavior when gzip=false or compressed=false (manual
decompression control) are not applicable; reference the HttpClient behavior in
src/HttpClient.ts around the undici auto-decompression logic (lines ~704-718) so
future readers know why these tests are skipped on Bun.
In `@test/options.opaque.test.ts`:
- Around line 35-53: Remove the full skip for Bun and ensure the test always
verifies that the opaque object is preserved on error: change the skipped test
(the it.skipIf(isBun) block around the 'should opaque work on error request'
case) to run unconditionally and inside the assert.rejects callback keep the
check assert.deepEqual(err.opaque, { traceId: 'some random id here' }) but make
the assertion on err.name/runtime-specific error conditional (e.g. if (isBun) {
assert.ok(['SomeBunErrorName', 'HTTPParserError'].includes(err.name)) } else {
assert.equal(err.name, 'HTTPParserError') }), keeping the other checks on
err.res.status and using the same urllib.request call with opaque to locate the
code.
In `@test/options.reset.test.ts`:
- Around line 29-31: The test currently only asserts connection === 'close'
inside the if (!isBun) branch, leaving Bun as a no-op; update the test in
options.reset.test.ts to add an else branch for when isBun is true that asserts
the response.data.headers.connection is not 'close' (or is undefined/null) so
the Bun behavior is validated; locate the isBun check and
response.data.headers.connection usage and add the complementary assertion in
the else branch to tighten the test.
In `@test/options.stream.test.ts`:
- Line 107: Add brief explanatory comments above each it.skipIf(isBun) test in
test/options.stream.test.ts (the tests with descriptions "should close 1KB
request stream when request timeout" and the other two at the same pattern) that
explain why they are skipped on Bun (e.g., missing stream timeout behavior,
platform-specific socket handling, or known Bun bug/limitation), mirroring the
style used in urllib.options.rejectUnauthorized-false.test.ts; place the comment
immediately above the it.skipIf call so future maintainers can quickly see the
Bun-specific reason and any issue/PR reference if available.
In `@test/options.streaming.test.ts`:
- Line 58: Add an inline comment next to the skipped test declaration (the
it.skipIf(isBun) for 'should work on streaming=true and compressed=true/false')
explaining that the test is skipped on Bun because Bun auto-decompresses
response bodies (see HttpClient behavior around src/HttpClient.ts:704-718),
which invalidates assertions about compressed=true/false; mention that this
causes streaming/compressed behavior differences on Bun and reference isBun as
the guard.
In `@test/options.timing.test.ts`:
- Around line 24-84: The two skipped tests ('should timing = true work' and
'should timing default to true' in test/options.timing.test.ts) lose Bun
coverage; update them so when isBun is true they perform a minimal Bun-friendly
contract: request with timing: true (or default) and assert response.status ===
200, res is RawResponseWithMeta, res.timing exists, each timing field (e.g.,
waiting, queuing, connected, requestHeadersSent, requestSent, contentDownload)
is >= 0 (not > 0), and res.rt > 0; keep the existing stricter assertions for
non-Bun runtimes (isBun false) inside the same test by branching on isBun so you
don't fully skip the tests.
In `@test/options.writeStream.test.ts`:
- Around line 52-159: Tests currently skip writeStream edge cases on Bun;
instead remove skipIf(isBun) and make Bun-specific assertions so behavior is
validated across runtimes. For each test named 'should work with
compressed=true/false', 'should close writeStream when request timeout', 'should
throw request error when writeStream error', and 'should end writeStream when
server error' replace the top-level skip with runtime branches using the isBun
flag: keep the same setup (createWriteStream, urllib.request) but assert
alternative expectations when isBun is true (e.g. different
destroyed/close/error semantics) and keep the original assertions for non-Bun.
Reference symbols: createWriteStream, urllib.request, writeStream.destroyed,
writeStream.on('close'), writeStream.on('error'), writeStreamClosed,
writeStreamError, and the existing process.version v16 check; implement concise
if (isBun) { /* Bun expectations */ } else { /* original asserts */ } blocks in
each test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 537989a4-5322-494c-8490-00d3ec246e18
📒 Files selected for processing (33)
src/FormData.tssrc/HttpClient.tssrc/fetch.tstest/HttpClient.connect.rejectUnauthorized.test.tstest/HttpClient.events.test.tstest/HttpClient.test.tstest/diagnostics_channel.test.tstest/fetch.test.tstest/formData-with-BufferStream.test.tstest/head-request-should-keepalive.test.tstest/index.test.tstest/keep-alive-header.test.tstest/non-ascii-request-header.test.tstest/options.compressed.test.tstest/options.content.test.tstest/options.data.test.tstest/options.dataType.test.tstest/options.fixJSONCtlChars.test.tstest/options.followRedirect.test.tstest/options.gzip.test.tstest/options.headers.test.tstest/options.opaque.test.tstest/options.reset.test.tstest/options.signal.test.tstest/options.socketErrorRetry.test.tstest/options.socketPath.test.tstest/options.stream.test.tstest/options.streaming.test.tstest/options.timeout.test.tstest/options.timing.test.tstest/options.writeStream.test.tstest/urllib.options.rejectUnauthorized-false.test.tstest/user-agent.test.ts
| function getRuntimeInfo(): string { | ||
| if (isBun) { | ||
| return `Bun/${process.versions.bun}`; | ||
| } | ||
| return `Node.js/${process.version.substring(1)}`; | ||
| } | ||
|
|
||
| // 'node-urllib/4.0.0 Node.js/18.19.0 (darwin; x64)' | ||
| export const HEADER_USER_AGENT: string = `node-urllib/${VERSION} Node.js/${process.version.substring(1)} (${process.platform}; ${process.arch})`; | ||
| // 'node-urllib/4.0.0 Bun/1.2.5 (darwin; x64)' | ||
| export const HEADER_USER_AGENT: string = `node-urllib/${VERSION} ${getRuntimeInfo()} (${process.platform}; ${process.arch})`; |
There was a problem hiding this comment.
Keep the canonical Node.js/... User-Agent token.
This replaces the repo-wide Node.js/<version> token with Bun/<version>, which is a behavior change for servers/tests that parse the existing UA format. If Bun needs to be surfaced, add it separately instead of changing the canonical value.
As per coding guidelines, **/*.ts: Set User-Agent header to node-urllib/{version} Node.js/{version} ({platform}; {arch}).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` around lines 117 - 126, The User-Agent construction
currently swaps the canonical "Node.js/..." token for "Bun/..." when running in
Bun; change getRuntimeInfo so it always returns the canonical Node.js token
(e.g. `Node.js/${process.version.substring(1)}`) and adjust HEADER_USER_AGENT to
append a separate Bun token only when running under Bun (e.g. add `
Bun/${process.versions.bun}` when isBun is true) so HEADER_USER_AGENT preserves
`node-urllib/${VERSION} Node.js/{version} (...)` and surfaces Bun separately;
update references in getRuntimeInfo and HEADER_USER_AGENT (and use VERSION)
accordingly.
src/HttpClient.ts
Outdated
| let requestSignal = args.signal; | ||
| if (isBun) { | ||
| const bunTimeoutSignal = AbortSignal.timeout(headersTimeout + bodyTimeout); | ||
| requestSignal = args.signal | ||
| ? AbortSignal.any([bunTimeoutSignal, args.signal]) | ||
| : bunTimeoutSignal; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "Bun timeout composition in src/HttpClient.ts:"
sed -n '443,448p' src/HttpClient.ts
echo
echo "RequestOptions.signal definition in src/Request.ts:"
rg -n -C2 '\bsignal\??:' src/Request.tsRepository: node-modules/urllib
Length of output: 546
AbortSignal.any() is blocking the build—type incompatibility at line 447.
Line 447 passes args.signal directly to AbortSignal.any(), but RequestOptions.signal is typed as AbortSignal | EventEmitter. The AbortSignal.any() method only accepts AbortSignal instances, not EventEmitter. When an EventEmitter is provided, TypeScript fails with TS2740. Narrow the signal type to AbortSignal before composing it with the timeout signal, or handle the EventEmitter case separately.
🧰 Tools
🪛 GitHub Actions: Node.js old versions CI
[error] 447-447: TypeScript (tsc) failed with TS2740: Type '{}' is missing required properties from type 'AbortSignal' (aborted, onabort, reason, throwIfAborted, and 3 more). Error location: AbortSignal.any([bunTimeoutSignal, args.signal]).
🪛 GitHub Actions: Publish Any Commit
[error] 447-447: TypeScript (tshy) compile error TS2740: Type '{}' is missing the following properties from type 'AbortSignal': aborted, onabort, reason, throwIfAborted, and 3 more. Error at 'AbortSignal.any([bunTimeoutSignal, args.signal])'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` around lines 443 - 448, The build fails because
AbortSignal.any() only accepts AbortSignal, but args.signal can be an
EventEmitter; update the Bun-specific block that creates requestSignal (around
isBun, AbortSignal.timeout, headersTimeout, bodyTimeout) to first narrow
args.signal to an AbortSignal (e.g., check typeof/instance or a type guard) and
only call AbortSignal.any([bunTimeoutSignal, args.signal]) when args.signal is
an AbortSignal; otherwise handle the EventEmitter case separately (for example,
use bunTimeoutSignal alone or convert/subscribe the EventEmitter without passing
it into AbortSignal.any).
| // Bun's CombinedStream can't pipe file streams | ||
| value = isBun ? await readFile(file) : createReadStream(file); | ||
| } else if (Buffer.isBuffer(file)) { |
There was a problem hiding this comment.
Please don't silently materialize every Bun multipart upload in memory.
This path reads file inputs with readFile(), drains readable uploads into Buffer.concat(), and then materializes the full multipart body via toBuffer(). Large uploads lose streaming backpressure and can OOM the process. A safer fallback is to reject unsupported Bun streaming uploads or enforce a size cap instead of buffering unboundedly.
Based on learnings, urllib is built as a Node.js HTTP client library on top of undici with features including basic/digest authentication, redirections, timeout handling, gzip/brotli compression, file uploads, and HTTP/2 support.
Also applies to: 528-535, 548-550
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` around lines 521 - 523, The code currently forces Bun
multipart streams into memory via readFile / Buffer.concat / toBuffer which can
OOM for large uploads; update the HttpClient upload handling (check isBun and
the multipart file/stream branches in the upload/multipart logic) to avoid
silently materializing streams: detect when a file input is a Readable/stream
and on Bun either (a) throw a clear, documented error rejecting streaming
uploads on Bun with guidance to provide a file path or a Buffer, or (b) enforce
a configurable size cap (e.g., maxUploadBytes) and reject if the stream exceeds
it; do not call readFile or Buffer.concat unconditionally for stream
inputs—either require file paths (use createReadStream) or write to a temporary
file on disk with streaming I/O before building the multipart body so
backpressure is preserved; ensure the change touches the
isBun/readFile/createReadStream/toBuffer/Buffer.concat code paths used for
multipart handling.
| if (isBun && args.content instanceof FormData) { | ||
| requestOptions.body = await (args.content as FormData).toBuffer(); | ||
| } else { | ||
| requestOptions.body = args.content; | ||
| } | ||
| if (args.contentType) { | ||
| headers['content-type'] = args.contentType; |
There was a problem hiding this comment.
Buffering Bun FormData needs to copy the multipart headers too.
Unlike the args.files branch, this path turns args.content into a bare Buffer without copying getHeaders(). Once the body is just bytes, the multipart boundary metadata is gone unless content-type is carried over here as well.
🐛 Proposed fix
if (isBun && args.content instanceof FormData) {
- requestOptions.body = await (args.content as FormData).toBuffer();
+ const formHeaders = args.content.getHeaders();
+ if (!headers['content-type'] && formHeaders['content-type']) {
+ headers['content-type'] = formHeaders['content-type'];
+ }
+ requestOptions.body = await args.content.toBuffer();
} 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 (isBun && args.content instanceof FormData) { | |
| requestOptions.body = await (args.content as FormData).toBuffer(); | |
| } else { | |
| requestOptions.body = args.content; | |
| } | |
| if (args.contentType) { | |
| headers['content-type'] = args.contentType; | |
| if (isBun && args.content instanceof FormData) { | |
| const formHeaders = args.content.getHeaders(); | |
| if (!headers['content-type'] && formHeaders['content-type']) { | |
| headers['content-type'] = formHeaders['content-type']; | |
| } | |
| requestOptions.body = await args.content.toBuffer(); | |
| } else { | |
| requestOptions.body = args.content; | |
| } | |
| if (args.contentType) { | |
| headers['content-type'] = args.contentType; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` around lines 557 - 563, When running in Bun and
args.content is a FormData, the code currently buffers it to a Buffer but
forgets to copy multipart headers (boundary) from FormData.getHeaders(), so the
content-type/boundary is lost; update the isBun branch where args.content is
handled (look for isBun, args.content, requestOptions.body and FormData casting)
to call (args.content as FormData).getHeaders() and merge its content-type into
the headers map before/after setting requestOptions.body so the multipart
boundary is preserved (ensure headers['content-type'] is set from getHeaders()
when present).
| headers['content-type'] = 'text/plain;charset=UTF-8'; | ||
| } | ||
| isStreamingRequest = isReadable(args.content); | ||
| isStreamingRequest = !isBun && isReadable(args.content); |
There was a problem hiding this comment.
Bun content streams still need the streaming retry/redirect guard.
Line 567 forces Bun args.content streams to look replayable, but Lines 620-621 still send them as a one-shot web stream. That means Lines 609-613 never disable redirect/socketErrorRetry for this path, so a 30x or retry can end up reusing a consumed body from options.content.
🔒 Proposed fix
- isStreamingRequest = !isBun && isReadable(args.content);
+ isStreamingRequest = isReadable(args.content);Also applies to: 609-613, 620-622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` at line 567, The code treats Bun stream bodies as
replayable by setting isStreamingRequest = !isBun && isReadable(args.content);
instead, detect readable streams regardless of isBun and ensure you disable
redirect and socketErrorRetry for those cases: update the isStreamingRequest
logic to consider isReadable(args.content) even when isBun, and in the
request-paths (the blocks referencing redirect and socketErrorRetry around where
options.content/args.content are used) explicitly turn off redirect and
socketErrorRetry when args.content or options.content is a readable stream so
streaming requests are not retried or redirected.
| } else if (isBun && err.name === 'TimeoutError') { | ||
| // Bun's undici throws TimeoutError instead of HeadersTimeoutError/BodyTimeoutError | ||
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); | ||
| } else if (isBun && err.name === 'TypeError' && /timed?\s*out|timeout/i.test(err.message)) { | ||
| // Bun may wrap timeout as TypeError | ||
| err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); |
There was a problem hiding this comment.
Report the timeout Bun actually enforced.
Line 818 wraps Bun timeouts with headersTimeout || bodyTimeout, but the fallback signal above aborts after headersTimeout + bodyTimeout. With split values, callers will see the wrong timeout in HttpClientRequestTimeoutError.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HttpClient.ts` around lines 816 - 821, The Bun timeout branches (checks
on isBun and err.name) currently construct HttpClientRequestTimeoutError using
headersTimeout || bodyTimeout, which hides the actual timeout when the code uses
the fallback abort signal that fires after headersTimeout + bodyTimeout; update
those branches to compute and pass the real enforced timeout (e.g., use
headersTimeout + bodyTimeout when both are set, otherwise use the one that
exists) into HttpClientRequestTimeoutError so the error reports the correct
timeout value (refer to isBun, the err.name checks,
HttpClientRequestTimeoutError, headersTimeout, and bodyTimeout to locate the
changes).
| if (isBun) { | ||
| // Bun's socket error code differs, retry may not trigger | ||
| assert(err.res.socketErrorRetries >= 0); | ||
| } else { | ||
| assert.equal(err.res.socketErrorRetries, 1); | ||
| } |
There was a problem hiding this comment.
Bun retry assertion is too weak to detect regressions.
At Line [47], assert(err.res.socketErrorRetries >= 0) is effectively non-validating. Please constrain the expected range so the test still verifies retry behavior.
Proposed assertion tightening
if (isBun) {
// Bun's socket error code differs, retry may not trigger
- assert(err.res.socketErrorRetries >= 0);
+ assert.equal(typeof err.res.socketErrorRetries, 'number');
+ assert(err.res.socketErrorRetries >= 0 && err.res.socketErrorRetries <= 1);
} else {
assert.equal(err.res.socketErrorRetries, 1);
}📝 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 (isBun) { | |
| // Bun's socket error code differs, retry may not trigger | |
| assert(err.res.socketErrorRetries >= 0); | |
| } else { | |
| assert.equal(err.res.socketErrorRetries, 1); | |
| } | |
| if (isBun) { | |
| // Bun's socket error code differs, retry may not trigger | |
| assert.equal(typeof err.res.socketErrorRetries, 'number'); | |
| assert(err.res.socketErrorRetries >= 0 && err.res.socketErrorRetries <= 1); | |
| } else { | |
| assert.equal(err.res.socketErrorRetries, 1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/options.socketErrorRetry.test.ts` around lines 45 - 50, The Bun branch
in the test uses a non-strict assertion (assert(err.res.socketErrorRetries >=
0)) which won't catch regressions; tighten it to validate retry behavior by
asserting a meaningful minimum or exact value for err.res.socketErrorRetries
when isBun is true (e.g., assert(err.res.socketErrorRetries >= 1) or
assert.equal(err.res.socketErrorRetries, expectedBunRetries)), updating the test
in options.socketErrorRetry.test.ts to use that stricter check alongside the
existing non-Bun assert.equal path so the socket retry behavior for the isBun
branch is actually verified.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/bun.yml (2)
50-51: Consider pinning vitest and vite versions for reproducible builds.Using
@latestmeans CI results could differ between runs if vitest or vite releases a breaking change. If the intent is to always test against the latest versions, this is fine—but if reproducibility is preferred, pin to specific versions.💡 Optional: Pin versions for reproducibility
- pnpm install -D vitest@latest vite@latest + pnpm install -D vitest@^3.0.0 vite@^6.0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bun.yml around lines 50 - 51, The CI step currently installs vitest and vite with the floating tags "pnpm install -D vitest@latest vite@latest"; change this to pin explicit versions (e.g., vitest@<x.y.z> and vite@<a.b.c>) or replace them with workflow variables/inputs so builds are reproducible; update the command string "pnpm install -D vitest@latest vite@latest" to use the chosen fixed version tokens (or env-based version variables) and ensure the workflow passes those versions into the install step.
24-37: Consider adding pnpm caching to speed up CI runs.The GitHub Actions versions used (
actions/checkout@v6andactions/setup-node@v6) are valid and current as of March 2026. However, the workflow is missing pnpm caching. Addcache: 'pnpm'to theactions/setup-nodestep to cache the pnpm store automatically.♻️ Suggested improvement with caching
- uses: actions/checkout@v6 - uses: oven-sh/setup-bun@v2 with: bun-version: latest - uses: actions/setup-node@v6 with: node-version: '22' + cache: 'pnpm' - uses: pnpm/action-setup@v4 - run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/bun.yml around lines 24 - 37, Add pnpm caching to the CI by updating the actions/setup-node step to enable the pnpm cache; specifically modify the setup node configuration (the "actions/setup-node@v6" step) to include cache: 'pnpm' so the pnpm store is cached between runs and pnpm install --frozen-lockfile runs faster. Ensure you only add the cache key to that step and keep the existing node-version and other steps unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/bun.yml:
- Around line 50-51: The CI step currently installs vitest and vite with the
floating tags "pnpm install -D vitest@latest vite@latest"; change this to pin
explicit versions (e.g., vitest@<x.y.z> and vite@<a.b.c>) or replace them with
workflow variables/inputs so builds are reproducible; update the command string
"pnpm install -D vitest@latest vite@latest" to use the chosen fixed version
tokens (or env-based version variables) and ensure the workflow passes those
versions into the install step.
- Around line 24-37: Add pnpm caching to the CI by updating the
actions/setup-node step to enable the pnpm cache; specifically modify the setup
node configuration (the "actions/setup-node@v6" step) to include cache: 'pnpm'
so the pnpm store is cached between runs and pnpm install --frozen-lockfile runs
faster. Ensure you only add the cache key to that step and keep the existing
node-version and other steps unchanged.
- Cast AbortSignal.any() to bypass TypeScript type check on older targets that don't include the type definition - Fix Bun CI vitest path: use node_modules/vitest/vitest.mjs instead of shell wrapper script that Bun can't parse - Fix formatting with oxfmt (import sorting, line width) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #717 +/- ##
==========================================
- Coverage 92.77% 89.91% -2.86%
==========================================
Files 10 10
Lines 747 793 +46
Branches 233 255 +22
==========================================
+ Hits 693 713 +20
- Misses 51 72 +21
- Partials 3 8 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
|
|
||
| - uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: '22' | ||
|
|
||
| - uses: pnpm/action-setup@v4 | ||
|
|
||
| - run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Patch vitest imports | ||
| run: | | ||
| for f in test/*.test.ts; do | ||
| [ -e "$f" ] || exit 0 | ||
| sed --version >/dev/null 2>&1 && \ | ||
| sed -i "s@'vite-plus/test'@'vitest'@g" "$f" || \ | ||
| sed -i '' "s@'vite-plus/test'@'vitest'@g" "$f" | ||
| done | ||
| sed --version >/dev/null 2>&1 && \ | ||
| sed -i "s@'vite-plus'@'vite'@g" vite.config.ts || \ | ||
| sed -i '' "s@'vite-plus'@'vite'@g" vite.config.ts | ||
| jq 'del(.overrides) | del(.pnpm.overrides)' package.json > package.json.tmp && mv package.json.tmp package.json | ||
| pnpm install -D vitest@latest vite@latest |
There was a problem hiding this comment.
CI uses floating versions (bun-version: latest and pnpm install -D vitest@latest vite@latest). This makes the workflow non-deterministic and can introduce sudden failures unrelated to repo changes. Consider pinning Bun (and the patched vitest/vite versions) to known-good versions (or a stable semver range) so CI results are reproducible.
| async toBuffer(): Promise<Buffer> { | ||
| return new Promise<Buffer>((resolve, reject) => { | ||
| const chunks: Buffer[] = []; | ||
| this.on('data', (chunk: Buffer | string) => { | ||
| // CombinedStream emits boundary/header strings alongside Buffer data | ||
| chunks.push(typeof chunk === 'string' ? Buffer.from(chunk) : chunk); | ||
| }); | ||
| this.on('end', () => resolve(Buffer.concat(chunks))); | ||
| this.on('error', reject); | ||
| // CombinedStream pauses by default and only starts | ||
| // flowing when piped or explicitly resumed | ||
| this.resume(); | ||
| }); |
There was a problem hiding this comment.
toBuffer() attaches data/end/error listeners with on(...) but never removes them. If toBuffer() is called more than once on the same instance, listeners will accumulate and the method may resolve with duplicated data or leak memory. Consider using once(...) (and/or removing listeners on resolve/reject) to make this method safe against repeated calls.
| let requestSignal = args.signal; | ||
| if (isBun) { | ||
| const bunTimeoutSignal = AbortSignal.timeout(headersTimeout + bodyTimeout); | ||
| requestSignal = args.signal ? (AbortSignal as any).any([bunTimeoutSignal, args.signal]) : bunTimeoutSignal; |
There was a problem hiding this comment.
On Bun, args.signal is allowed to be an EventEmitter (per RequestOptions.signal), but this code unconditionally feeds it into AbortSignal.any(...). If a caller passes an EventEmitter signal under Bun, this will throw synchronously (since AbortSignal.any expects AbortSignal instances), changing behavior from a request rejection to a runtime error. Consider detecting non-AbortSignal values when isBun and either (a) ignore/omit the user signal, or (b) throw a clear, early validation error explaining that Bun only supports AbortSignal for signal.
| requestSignal = args.signal ? (AbortSignal as any).any([bunTimeoutSignal, args.signal]) : bunTimeoutSignal; | |
| if (args.signal == null) { | |
| // No user-provided signal: use only the timeout-based AbortSignal | |
| requestSignal = bunTimeoutSignal; | |
| } else if (args.signal instanceof AbortSignal) { | |
| // User provided an AbortSignal: safely combine it with the timeout signal | |
| requestSignal = (AbortSignal as any).any([bunTimeoutSignal, args.signal]); | |
| } else { | |
| // On Bun, RequestOptions.signal may be an EventEmitter or other type. | |
| // Pass it through as-is to avoid AbortSignal.any throwing at runtime. | |
| requestSignal = args.signal; | |
| } |
| if (isBun && requestOptions.body instanceof Readable) { | ||
| requestOptions.body = Readable.toWeb(requestOptions.body) as any; |
There was a problem hiding this comment.
The Bun body conversion only runs when requestOptions.body instanceof Readable. For “old-style” streams that satisfy isReadable(...) but are not instanceof Readable (which this codebase already supports elsewhere via isReadable/Readable().wrap(...)), Bun will still receive a Node stream and likely fail. Consider changing this guard to use isReadable(requestOptions.body) and wrapping non-Readable streams before calling Readable.toWeb(...).
| if (isBun && requestOptions.body instanceof Readable) { | |
| requestOptions.body = Readable.toWeb(requestOptions.body) as any; | |
| if (isBun && isReadable(requestOptions.body)) { | |
| const nodeReadable = | |
| requestOptions.body instanceof Readable | |
| ? requestOptions.body | |
| : new Readable({ read() {} }).wrap(requestOptions.body as any); | |
| requestOptions.body = Readable.toWeb(nodeReadable) as any; |
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/HttpClient.ts (6)
555-556:⚠️ Potential issue | 🔴 CriticalMerge the multipart boundary headers before buffering
FormData.Lines 555-556 turn
args.contentinto raw bytes, but this branch never copiesargs.content.getHeaders(). Without the generatedcontent-typeboundary, caller-suppliedFormDatarequests are malformed unless the caller manually sets the header.💡 Suggested fix
if (isBun && args.content instanceof FormData) { - requestOptions.body = await (args.content as FormData).toBuffer(); + const formHeaders = args.content.getHeaders(); + if (!headers['content-type'] && formHeaders['content-type']) { + headers['content-type'] = formHeaders['content-type']; + } + requestOptions.body = await args.content.toBuffer(); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` around lines 555 - 556, The Bun branch currently converts args.content (FormData) to a Buffer but doesn't include the multipart headers produced by args.content.getHeaders(), so the boundary is lost; update the isBun handling (where args.content is checked and requestOptions.body is set) to merge the headers from (args.content as FormData).getHeaders() into requestOptions.headers (preserving any existing headers, lower/upper-case normalization as needed) before or alongside setting requestOptions.body so the Content-Type with boundary is sent with the buffered body.
117-126:⚠️ Potential issue | 🟡 MinorKeep the canonical
Node.js/...token in the User-Agent.On Bun this replaces the stable
Node.js/<version>segment withBun/<version>, which changes the header contract for callers that parse or snapshot the existing format. KeepNode.js/${process.version.substring(1)}in the main slot and append Bun separately if you still want runtime visibility.As per coding guidelines,
**/*.ts: Set User-Agent header tonode-urllib/{version} Node.js/{version} ({platform}; {arch}).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` around lines 117 - 126, The User-Agent currently substitutes the canonical "Node.js/{version}" token on Bun via getRuntimeInfo(), which breaks consumers that expect "Node.js/{version}"; change the construction so HEADER_USER_AGENT always contains "Node.js/{process.version.substring(1)}" in the main slot and, if isBun is true, append a separate "Bun/{process.versions.bun}" token afterwards. Update getRuntimeInfo() or the HEADER_USER_AGENT expression to reference process.version.substring(1) for Node and conditionally add the Bun token (using isBun and process.versions.bun) rather than replacing Node.js.
814-819:⚠️ Potential issue | 🟡 MinorReport the timeout Bun actually enforced.
The Bun fallback signal is created with
headersTimeout + bodyTimeout, but Lines 816 and 819 still surfaceheadersTimeout || bodyTimeout. With split values,HttpClientRequestTimeoutErrorreports the wrong deadline.💡 Suggested fix
+ const bunEnforcedTimeout = headersTimeout + bodyTimeout; if (err.name === 'HeadersTimeoutError') { err = new HttpClientRequestTimeoutError(headersTimeout, { cause: err }); } else if (err.name === 'BodyTimeoutError') { err = new HttpClientRequestTimeoutError(bodyTimeout, { cause: err }); } else if (err.name === 'InformationalError' && err.message.includes('stream timeout')) { err = new HttpClientRequestTimeoutError(bodyTimeout, { cause: err }); } else if (isBun && err.name === 'TimeoutError') { // Bun's undici throws TimeoutError instead of HeadersTimeoutError/BodyTimeoutError - err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); + err = new HttpClientRequestTimeoutError(bunEnforcedTimeout, { cause: err }); } else if (isBun && err.name === 'TypeError' && /timed?\s*out|timeout/i.test(err.message)) { // Bun may wrap timeout as TypeError - err = new HttpClientRequestTimeoutError(headersTimeout || bodyTimeout, { cause: err }); + err = new HttpClientRequestTimeoutError(bunEnforcedTimeout, { cause: err });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` around lines 814 - 819, The timeout branches for Bun currently construct HttpClientRequestTimeoutError using headersTimeout || bodyTimeout which misreports the actual deadline when the fallback signal used headersTimeout + bodyTimeout; update the two Bun branches (the err.name === 'TimeoutError' and the TypeError with timed out message) to compute and pass the real enforced timeout (e.g., use (headersTimeout ?? 0) + (bodyTimeout ?? 0) or a dedicated computed variable) into the HttpClientRequestTimeoutError constructor instead of headersTimeout || bodyTimeout so the error reflects the combined deadline. Ensure you reference the existing HttpClientRequestTimeoutError creation sites so only those two assignments are changed.
443-446:⚠️ Potential issue | 🟠 MajorNarrow
args.signalbefore callingAbortSignal.any().Line 446 still passes the public
RequestOptions.signalvalue straight intoAbortSignal.any(). Theas anycast only hides the type error; if that option is still allowed to be anEventEmitter, Bun will fail at runtime instead of composing the signal.Run this to confirm whether
RequestOptions.signalis still broader thanAbortSignal. If it is, this branch needs a runtime guard before composing the signals.#!/bin/bash set -e echo "RequestOptions.signal declaration:" rg -n -C2 '\bsignal\??:' src/Request.ts echo echo "Bun timeout composition:" sed -n '440,447p' src/HttpClient.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` around lines 443 - 446, The branch that composes a Bun timeout with the incoming signal is using the public RequestOptions.signal (args.signal) directly in AbortSignal.any — you must narrow it at runtime before composing to avoid passing non-AbortSignal types (like EventEmitter) into AbortSignal.any; modify the isBun branch around bunTimeoutSignal and requestSignal so you check that args.signal is an AbortSignal (e.g., typeof (args.signal as AbortSignal).aborted === 'boolean' or args.signal instanceof AbortSignal) before calling AbortSignal.any, and if it is not an AbortSignal just use bunTimeoutSignal (or otherwise wrap/convert safely) so AbortSignal.any is only ever passed AbortSignal instances (refer to args.signal, bunTimeoutSignal, requestSignal, and AbortSignal.any).
565-565:⚠️ Potential issue | 🔴 CriticalTreat Bun
Readablebodies as streaming requests too.Line 565 clears
isStreamingRequestfor Bun, but Lines 618-620 still send the same one-shot body as a web stream. Redirects and socket-error retries remain enabled, so a replay can hit a consumed request body.As per coding guidelines,
**/*.ts: Disable retry and redirect functionality for streaming requests.💡 Suggested fix
- isStreamingRequest = !isBun && isReadable(args.content); + isStreamingRequest = isReadable(args.content);Also applies to: 617-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` at line 565, The current check clears isStreamingRequest for Bun by using "isStreamingRequest = !isBun && isReadable(args.content)" which causes Bun Readable bodies to be treated as non-streaming and leaves retries/redirects enabled; change the logic to treat any Readable body as streaming regardless of isBun (i.e., base isStreamingRequest solely on isReadable(args.content)), and update the subsequent body-handling branch (the code around the send-as-web-stream path referenced near the lines 617-620) to respect isStreamingRequest so that retry and redirect behavior is disabled for those cases; refer to the symbols isStreamingRequest, isBun, isReadable, and args.content when making the change.
519-520:⚠️ Potential issue | 🟠 MajorAvoid unbounded buffering for Bun multipart uploads.
This path now reads file inputs into memory, drains readable uploads into
Buffer.concat(), and then materializes the full multipart body again viatoBuffer(). Large uploads lose backpressure and can OOM the process. Prefer a clear Bun-only rejection for readable multipart uploads or enforce a size cap before buffering.Also applies to: 526-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HttpClient.ts` around lines 519 - 520, The current Bun branch buffers readable file streams into memory (using isBun -> readFile and later toBuffer/Buffer.concat) causing unbounded memory use; update the multipart handling (symbols: isBun, readFile, createReadStream, value, toBuffer, CombinedStream) to either (A) explicitly reject readable/stream inputs when running under Bun with a clear error message, or (B) enforce a strict size cap before buffering and stream to a temporary file instead of accumulating via Buffer.concat, ensuring backpressure is preserved; modify the code paths that convert file streams for multipart assembly to implement one of these two strategies and surface a deterministic error for unsupported stream uploads on Bun.
🧹 Nitpick comments (3)
test/diagnostics_channel.test.ts (1)
18-18: Don't skip theurllib:*diagnostics assertions on Bun.This blanket suite skip also removes coverage for the library-owned
urllib:requestandurllib:responsechannels, even thoughsrc/HttpClient.tsstill publishes them directly. Consider splitting the suite so only the undici-internal channel cases are Bun-gated.As per coding guidelines,
**/*.ts: Publish request/response events viadiagnostics_channelwith event namesurllib:requestandurllib:response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/diagnostics_channel.test.ts` at line 18, The suite currently uses describe.skipIf(isBun) which skips all tests including the library-owned diagnostics assertions; change diagnostics_channel.test.ts to split the suite so only undici-internal channel tests are wrapped with describe.skipIf(isBun) while leaving the urllib channel assertions active, and ensure tests still assert that src/HttpClient.ts publishes events on diagnostics_channel with event names "urllib:request" and "urllib:response"; locate the tests referencing describe.skipIf(isBun) and the assertions for "urllib:request"/"urllib:response" and move only the undici-internal cases into the Bun-gated describe so urllib assertions run on Bun.test/options.fixJSONCtlChars.test.ts (1)
34-34: Prefer a Bun-specific assertion over skipping this parse-error test.This request still goes through the same
parseJSON()/JSONResponseFormatErrorpath insrc/HttpClient.ts. If Bun only changes the parser text, branch the message assertion instead of dropping Bun coverage for the error contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.fixJSONCtlChars.test.ts` at line 34, The test currently skips on Bun; instead run it everywhere and assert the error shape but branch the expected message based on the environment: call the same test ('should throw error when json control characters exists'), catch the thrown error from parseJSON()/HttpClient code, ensure it's an instance of JSONResponseFormatError, and assert its message contains the non-Bun substring in the normal case and the Bun-specific parser substring when isBun is true (use the isBun flag to decide which substring to expect). This keeps the contract check (JSONResponseFormatError) while allowing the message assertion to differ for Bun.test/options.content.test.ts (1)
166-166: Keep Bun coverage forcontent: Readable.
src/HttpClient.tsnow has a Bun-specificReadable.toWeb()branch for this exact request shape. Skipping the test means Bun CI no longer exercises that compatibility path; if the wire-level headers differ, branch those assertions instead of disabling the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/options.content.test.ts` at line 166, The test "should POST content = readable" has been skipped for Bun (it.skipIf(isBun)), which prevents exercising the Bun-specific Readable.toWeb path in src/HttpClient.ts; remove the skip so Bun runs the test, and instead update the test's assertions to branch or accept both header shapes (e.g., detect isBun and assert the Bun-specific wire-level headers or use a flexible matcher) so the test passes across runtimes while still exercising the Readable.toWeb compatibility branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/HttpClient.ts`:
- Around line 555-556: The Bun branch currently converts args.content (FormData)
to a Buffer but doesn't include the multipart headers produced by
args.content.getHeaders(), so the boundary is lost; update the isBun handling
(where args.content is checked and requestOptions.body is set) to merge the
headers from (args.content as FormData).getHeaders() into requestOptions.headers
(preserving any existing headers, lower/upper-case normalization as needed)
before or alongside setting requestOptions.body so the Content-Type with
boundary is sent with the buffered body.
- Around line 117-126: The User-Agent currently substitutes the canonical
"Node.js/{version}" token on Bun via getRuntimeInfo(), which breaks consumers
that expect "Node.js/{version}"; change the construction so HEADER_USER_AGENT
always contains "Node.js/{process.version.substring(1)}" in the main slot and,
if isBun is true, append a separate "Bun/{process.versions.bun}" token
afterwards. Update getRuntimeInfo() or the HEADER_USER_AGENT expression to
reference process.version.substring(1) for Node and conditionally add the Bun
token (using isBun and process.versions.bun) rather than replacing Node.js.
- Around line 814-819: The timeout branches for Bun currently construct
HttpClientRequestTimeoutError using headersTimeout || bodyTimeout which
misreports the actual deadline when the fallback signal used headersTimeout +
bodyTimeout; update the two Bun branches (the err.name === 'TimeoutError' and
the TypeError with timed out message) to compute and pass the real enforced
timeout (e.g., use (headersTimeout ?? 0) + (bodyTimeout ?? 0) or a dedicated
computed variable) into the HttpClientRequestTimeoutError constructor instead of
headersTimeout || bodyTimeout so the error reflects the combined deadline.
Ensure you reference the existing HttpClientRequestTimeoutError creation sites
so only those two assignments are changed.
- Around line 443-446: The branch that composes a Bun timeout with the incoming
signal is using the public RequestOptions.signal (args.signal) directly in
AbortSignal.any — you must narrow it at runtime before composing to avoid
passing non-AbortSignal types (like EventEmitter) into AbortSignal.any; modify
the isBun branch around bunTimeoutSignal and requestSignal so you check that
args.signal is an AbortSignal (e.g., typeof (args.signal as AbortSignal).aborted
=== 'boolean' or args.signal instanceof AbortSignal) before calling
AbortSignal.any, and if it is not an AbortSignal just use bunTimeoutSignal (or
otherwise wrap/convert safely) so AbortSignal.any is only ever passed
AbortSignal instances (refer to args.signal, bunTimeoutSignal, requestSignal,
and AbortSignal.any).
- Line 565: The current check clears isStreamingRequest for Bun by using
"isStreamingRequest = !isBun && isReadable(args.content)" which causes Bun
Readable bodies to be treated as non-streaming and leaves retries/redirects
enabled; change the logic to treat any Readable body as streaming regardless of
isBun (i.e., base isStreamingRequest solely on isReadable(args.content)), and
update the subsequent body-handling branch (the code around the
send-as-web-stream path referenced near the lines 617-620) to respect
isStreamingRequest so that retry and redirect behavior is disabled for those
cases; refer to the symbols isStreamingRequest, isBun, isReadable, and
args.content when making the change.
- Around line 519-520: The current Bun branch buffers readable file streams into
memory (using isBun -> readFile and later toBuffer/Buffer.concat) causing
unbounded memory use; update the multipart handling (symbols: isBun, readFile,
createReadStream, value, toBuffer, CombinedStream) to either (A) explicitly
reject readable/stream inputs when running under Bun with a clear error message,
or (B) enforce a strict size cap before buffering and stream to a temporary file
instead of accumulating via Buffer.concat, ensuring backpressure is preserved;
modify the code paths that convert file streams for multipart assembly to
implement one of these two strategies and surface a deterministic error for
unsupported stream uploads on Bun.
---
Nitpick comments:
In `@test/diagnostics_channel.test.ts`:
- Line 18: The suite currently uses describe.skipIf(isBun) which skips all tests
including the library-owned diagnostics assertions; change
diagnostics_channel.test.ts to split the suite so only undici-internal channel
tests are wrapped with describe.skipIf(isBun) while leaving the urllib channel
assertions active, and ensure tests still assert that src/HttpClient.ts
publishes events on diagnostics_channel with event names "urllib:request" and
"urllib:response"; locate the tests referencing describe.skipIf(isBun) and the
assertions for "urllib:request"/"urllib:response" and move only the
undici-internal cases into the Bun-gated describe so urllib assertions run on
Bun.
In `@test/options.content.test.ts`:
- Line 166: The test "should POST content = readable" has been skipped for Bun
(it.skipIf(isBun)), which prevents exercising the Bun-specific Readable.toWeb
path in src/HttpClient.ts; remove the skip so Bun runs the test, and instead
update the test's assertions to branch or accept both header shapes (e.g.,
detect isBun and assert the Bun-specific wire-level headers or use a flexible
matcher) so the test passes across runtimes while still exercising the
Readable.toWeb compatibility branch.
In `@test/options.fixJSONCtlChars.test.ts`:
- Line 34: The test currently skips on Bun; instead run it everywhere and assert
the error shape but branch the expected message based on the environment: call
the same test ('should throw error when json control characters exists'), catch
the thrown error from parseJSON()/HttpClient code, ensure it's an instance of
JSONResponseFormatError, and assert its message contains the non-Bun substring
in the normal case and the Bun-specific parser substring when isBun is true (use
the isBun flag to decide which substring to expect). This keeps the contract
check (JSONResponseFormatError) while allowing the message assertion to differ
for Bun.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e225abe-8968-4a3e-92fa-e34e744afda9
📒 Files selected for processing (31)
.github/workflows/bun.ymlsrc/HttpClient.tstest/HttpClient.connect.rejectUnauthorized.test.tstest/HttpClient.events.test.tstest/HttpClient.test.tstest/diagnostics_channel.test.tstest/fetch.test.tstest/head-request-should-keepalive.test.tstest/index.test.tstest/keep-alive-header.test.tstest/non-ascii-request-header.test.tstest/options.compressed.test.tstest/options.content.test.tstest/options.data.test.tstest/options.dataType.test.tstest/options.fixJSONCtlChars.test.tstest/options.followRedirect.test.tstest/options.gzip.test.tstest/options.headers.test.tstest/options.opaque.test.tstest/options.reset.test.tstest/options.signal.test.tstest/options.socketErrorRetry.test.tstest/options.socketPath.test.tstest/options.stream.test.tstest/options.streaming.test.tstest/options.timeout.test.tstest/options.timing.test.tstest/options.writeStream.test.tstest/urllib.options.rejectUnauthorized-false.test.tstest/user-agent.test.ts
✅ Files skipped from review due to trivial changes (10)
- test/HttpClient.connect.rejectUnauthorized.test.ts
- test/options.reset.test.ts
- test/options.followRedirect.test.ts
- test/options.headers.test.ts
- test/fetch.test.ts
- test/keep-alive-header.test.ts
- test/index.test.ts
- test/HttpClient.test.ts
- test/options.compressed.test.ts
- test/options.gzip.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- test/options.opaque.test.ts
- test/options.dataType.test.ts
- test/head-request-should-keepalive.test.ts
- test/options.timing.test.ts
- test/options.signal.test.ts
- test/urllib.options.rejectUnauthorized-false.test.ts
- test/user-agent.test.ts
- test/options.data.test.ts
- .github/workflows/bun.yml
- test/options.socketErrorRetry.test.ts
- test/options.streaming.test.ts
|
趁这个改动直接将 nodejs 最低版本也升级到 v22？ 可以发一个 major 变更 |
|
我先改一下发布流程，改成手动发布 |
Summary
AbortSignal.any()to compose Bun timeout signal with user-provided abort signalFormData.toBuffer()to serialize multipart form data for Bun's undicirejectUnauthorizedconnect option)Test results:
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
toBuffer()async method to FormData for buffer conversion.Improvements
Tests