Revert "fix: patch undici File support on Node.js < 20 (#588)" #592
Conversation
This reverts commit 0e5ad19.
WalkthroughDisables undici’s automatic redirect handling by setting maxRedirections: 0 and relies on existing manual redirect logic. Removes Node 16 compatibility shims from utils. Cleans minor whitespace in index. Updates tests to use default selfsigned cert generation and re-enables a multipart Content-Type assertion. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Caller
participant HC as HttpClient
participant U as undici
App->>HC: request(url, options)
HC->>U: request(url, { maxRedirections: 0, ... })
U-->>HC: response (no auto-redirect)
alt Is redirect status and redirects < maxRedirects
HC->>HC: resolve redirect URL, update context
HC->>U: request(nextUrl, { maxRedirections: 0, ... })
U-->>HC: response
end
HC-->>App: final response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request reverts commit 0e5ad19, which aimed to fix undici File support on Node.js versions less than 20. The revert removes specific polyfills and related imports, and also streamlines test certificate generation by removing Node.js version-dependent key size logic.
Highlights
- Revert of undici File support patch: The primary change is undoing the previous commit that introduced polyfills for global.File, String.prototype.toWellFormed, and String.prototype.isWellFormed to support undici on Node.js versions below 20.
- Cleanup of HttpClient configuration: The maxRedirections: 0 option in HttpClient.ts is re-enabled by uncommenting, restoring its intended behavior.
- Test suite simplification: The test files have been updated to remove nodeMajorVersion utility imports and usage, simplifying the selfsigned.generate() calls by removing conditional keySize settings.
- Restoration of test assertion: An assertion related to multipart/form-data content type in test/options.stream.test.ts has been uncommented, restoring its validation.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request reverts commit 0e5ad19, which had introduced a polyfill for File support on Node.js versions below 20. The changes effectively remove this polyfill and associated code, including the removal of File and toUSVString from imports in src/utils.ts and the deletion of the polyfill logic itself. Test files have been updated to remove dependencies on a nodeMajorVersion utility, simplifying certificate generation. A test case for multipart form data has also been re-enabled. The revert appears clean and correctly implemented.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils.ts (1)
5-5: Confirm intent: Node 16 polyfills still presentThis import is only used by patchForNode16() to polyfill global.Blob. If the goal is to drop Node 16 shims as part of this revert, consider removing patchForNode16() and this import; otherwise, keeping them is fine but please confirm the intended compatibility story.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/HttpClient.ts(1 hunks)src/index.ts(0 hunks)src/utils.ts(1 hunks)test/HttpClient.test.ts(1 hunks)test/diagnostics_channel.test.ts(1 hunks)test/fixtures/server.ts(2 hunks)test/options.stream.test.ts(1 hunks)test/options.timeout.test.ts(1 hunks)test/urllib.options.rejectUnauthorized-false.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/options.stream.test.ts (1)
test/cjs/index.js (1)
assert(2-2)
🪛 GitHub Actions: Node.js 16 CI
src/HttpClient.ts
[error] 432-432: TS2353: Object literal may only specify known properties, and 'maxRedirections' does not exist in type 'IUndiciRequestOption'.
🪛 GitHub Actions: Publish Any Commit
src/HttpClient.ts
[error] 432-432: TypeScript error TS2353 during npm run build: Object literal may only specify known properties, and 'maxRedirections' does not exist in type 'IUndiciRequestOption'.
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------86442430eab6492fe5b8831b'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------86442430eab6492fe5b8831b"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 24)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------2fdf9d79bde30b3b3cd5ae8d'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------2fdf9d79bde30b3b3cd5ae8d"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (macos-latest, 24)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------c32713078e5e011b81e48fe6'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------c32713078e5e011b81e48fe6"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------c8cec534105c72d9cff8a19a'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------c8cec534105c72d9cff8a19a"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------aad8037a0afa31756ffbeaaf'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------aad8037a0afa31756ffbeaaf"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
test/options.stream.test.ts
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------8c01933016ec59398b3dc4ed'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------8c01933016ec59398b3dc4ed"
❯ test/options.stream.test.ts:97:12
⏰ 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). (3)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 24)
🔇 Additional comments (6)
test/urllib.options.rejectUnauthorized-false.test.ts (1)
32-32: LGTM: switch to default selfsigned.generate()Using the library defaults simplifies the test setup and keeps behavior consistent across Node versions.
test/HttpClient.test.ts (1)
12-12: LGTM: TLS test setup simplifiedUsing
selfsigned.generate()defaults eliminates Node-version branching without affecting test intent.test/options.timeout.test.ts (1)
9-9: LGTM: default TLS generationConsistent with other tests; avoids brittle key-size branching.
test/diagnostics_channel.test.ts (1)
146-146: LGTM: default self-signed certs for H2 testAligns with the broader move to defaults; behavior remains unchanged.
test/fixtures/server.ts (2)
11-11: LGTM: import reorderingNo functional impact; maintainers might want to keep imports grouped consistently, but this is fine.
373-377: LGTM: default self-signed certs for HTTPS serverRemoves Node-version-dependent key-size logic and keeps fixture consistent with other tests.
| method, | ||
| // disable undici auto redirect handler | ||
| // maxRedirections: 0, | ||
| maxRedirections: 0, |
There was a problem hiding this comment.
TS2353: maxRedirections is not in undici typings across versions
CI fails with TS2353 because some undici typings don’t include maxRedirections. We still want to disable undici’s built-in redirects when supported. Assign the property post-creation with a type escape to satisfy all supported undici versions.
Apply this diff:
const requestOptions: IUndiciRequestOption = {
method,
- // disable undici auto redirect handler
- maxRedirections: 0,
headersTimeout,
headers,
bodyTimeout,
opaque: internalOpaque,
dispatcher: args.dispatcher ?? this.#dispatcher,
signal: args.signal,
reset: false,
};
+ // Disable undici auto redirect handling when supported by the runtime undici version.
+ // Some undici typings (e.g., v7) don’t declare `maxRedirections`, so set it via a type-escape.
+ // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+ // @ts-expect-error: 'maxRedirections' is not declared in some undici versions' types
+ (requestOptions as any).maxRedirections = 0;📝 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.
| maxRedirections: 0, | |
| const requestOptions: IUndiciRequestOption = { | |
| method, | |
| headersTimeout, | |
| headers, | |
| bodyTimeout, | |
| opaque: internalOpaque, | |
| dispatcher: args.dispatcher ?? this.#dispatcher, | |
| signal: args.signal, | |
| reset: false, | |
| }; | |
| // Disable undici auto redirect handling when supported by the runtime undici version. | |
| // Some undici typings (e.g., v7) don’t declare `maxRedirections`, so set it via a type-escape. | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |
| // @ts-expect-error: 'maxRedirections' is not declared in some undici versions' types | |
| (requestOptions as any).maxRedirections = 0; |
🧰 Tools
🪛 GitHub Actions: Node.js 16 CI
[error] 432-432: TS2353: Object literal may only specify known properties, and 'maxRedirections' does not exist in type 'IUndiciRequestOption'.
🪛 GitHub Actions: Publish Any Commit
[error] 432-432: TypeScript error TS2353 during npm run build: Object literal may only specify known properties, and 'maxRedirections' does not exist in type 'IUndiciRequestOption'.
🤖 Prompt for AI Agents
In src/HttpClient.ts around line 432, the undici option maxRedirections is
causing TS2353 because some undici typings don’t include it; remove it from the
initial options and instead, immediately after creating the undici client,
assign the property on the client instance using a type escape (cast the client
to any/unknown and set .maxRedirections = 0) so the runtime disables built-in
redirects when supported while satisfying all TypeScript versions.
| assert.equal(response.data.method, 'POST'); | ||
| // console.log(response.data); | ||
| // assert.match(response.data.headers['content-type'], /^multipart\/form-data; boundary=--------------------------\d+$/); | ||
| assert.match(response.data.headers['content-type'], /^multipart\/form-data; boundary=--------------------------\d+$/); |
There was a problem hiding this comment.
Fix failing boundary assertion (boundary includes hex chars, not only digits)
CI shows the boundary contains a hex string (0-9a-f), so the digits-only regex is too strict.
Apply this diff to make the assertion robust:
- assert.match(response.data.headers['content-type'], /^multipart\/form-data; boundary=--------------------------\d+$/);
+ // boundary is a crypto-ish hex string; allow hex chars instead of digits only
+ assert.match(response.data.headers['content-type'], /^multipart\/form-data; boundary=--------------------------[0-9a-f]+$/i);📝 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.
| assert.match(response.data.headers['content-type'], /^multipart\/form-data; boundary=--------------------------\d+$/); | |
| // boundary is a crypto-ish hex string; allow hex chars instead of digits only | |
| assert.match( | |
| response.data.headers['content-type'], | |
| /^multipart\/form-data; boundary=--------------------------[0-9a-f]+$/i | |
| ); |
🧰 Tools
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------86442430eab6492fe5b8831b'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------86442430eab6492fe5b8831b"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 24)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------2fdf9d79bde30b3b3cd5ae8d'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------2fdf9d79bde30b3b3cd5ae8d"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (macos-latest, 24)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------c32713078e5e011b81e48fe6'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------c32713078e5e011b81e48fe6"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------c8cec534105c72d9cff8a19a'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------c8cec534105c72d9cff8a19a"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------aad8037a0afa31756ffbeaaf'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------aad8037a0afa31756ffbeaaf"
❯ test/options.stream.test.ts:97:12
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 97-97: test/options.stream.test.ts > options.stream.test.ts > should upload file with formstream
AssertionError: The input did not match the regular expression /^multipart/form-data; boundary=--------------------------\d+$/. Input:
'multipart/form-data; boundary=--------------------------8c01933016ec59398b3dc4ed'
- Expected:
/^multipart/form-data; boundary=--------------------------\d+$/
- Received:
"multipart/form-data; boundary=--------------------------8c01933016ec59398b3dc4ed"
❯ test/options.stream.test.ts:97:12
🤖 Prompt for AI Agents
In test/options.stream.test.ts around line 97, the current assertion expects the
multipart boundary to be only digits which is incorrect because CI shows the
boundary includes hex characters; update the regex to allow hexadecimal
characters (e.g. use [0-9a-fA-F]+ or add the /i flag) so the assertion matches
boundaries containing letters a–f as well as digits.
This reverts commit 0e5ad19.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests