feat: use undici v6 #514
feat: use undici v6 #514
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 16 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe updates involve modernizing dependencies, refactoring modules, and enhancing configurations to support newer Node.js versions. Major changes include updating to Node.js versions 18, 20, and 22, refining import syntax, removing outdated patterns, and improving error handling. The documentation and configuration policies were also aligned to reflect these enhancements, ensuring higher compatibility and streamlined processes. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/[email protected], npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
src/utils.ts (1)
Line range hint
126-126: Use modern exponentiation operator.Replace
Math.powwith the exponentiation operator**for better readability and modern JavaScript practices.- const MAX_ID_VALUE = Math.pow(2, 31) - 10; + const MAX_ID_VALUE = (2 ** 31) - 10;src/HttpClient.ts (3)
Line range hint
354-354: Consider alternative todeletefor performance.Using the
deleteoperator can lead to performance issues due to changes in the underlying object's shape. Consider setting the property toundefinedor restructuring the object if feasible.- delete headers['user-agent']; + headers['user-agent'] = undefined;Tools
GitHub Check: Node.js / Test (windows-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
Line range hint
546-546: Use optional chaining for safer access.Changing to optional chaining can prevent potential runtime errors when accessing properties on
undefinedornullobjects.- if (socket[symbols.kSocketLocalAddress]) { + if (socket?.[symbols.kSocketLocalAddress]) {Tools
GitHub Check: Node.js / Test (windows-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
Line range hint
562-562: Correct unsafe usage of optional chaining.Ensure that the use of optional chaining does not lead to a TypeError when it short-circuits with
undefined.- const socket = internalOpaque[symbols.kRequestSocket] ?? err?.[symbols.kErrorSocket]; + const socket = internalOpaque[symbols.kRequestSocket] ?? (err ? err[symbols.kErrorSocket] : undefined);Tools
GitHub Check: Node.js / Test (windows-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (windows-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 22)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freeGitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (18)
- .eslintrc (1 hunks)
- .github/workflows/nodejs.yml (1 hunks)
- README.md (1 hunks)
- package.json (3 hunks)
- scripts/replace_urllib_version.js (2 hunks)
- src/HttpClient.ts (8 hunks)
- src/diagnosticsChannel.ts (8 hunks)
- src/index.ts (1 hunks)
- src/utils.ts (2 hunks)
- test/diagnostics_channel.test.ts (1 hunks)
- test/fixtures/server.ts (1 hunks)
- test/keep-alive-header.test.ts (2 hunks)
- test/options.signal.test.ts (1 hunks)
- test/options.timeout.test.ts (1 hunks)
- test/options.timing.test.ts (1 hunks)
- test/options.writeStream.test.ts (1 hunks)
- test/utils.ts (1 hunks)
- tsconfig.json (1 hunks)
Files skipped from review due to trivial changes (6)
- .eslintrc
- .github/workflows/nodejs.yml
- package.json
- test/options.timeout.test.ts
- test/utils.ts
- tsconfig.json
Additional context used
Biome
src/utils.ts
[error] 16-16: Unexpected control character(s) in regular expression: \u0000, \u001F (lint/suspicious/noControlCharactersInRegex)
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
[error] 126-126: Use the '**' operator instead of 'Math.pow'. (lint/style/useExponentiationOperator)
Unsafe fix: Use the '**' operator instead of 'Math.pow'.
src/HttpClient.ts
[error] 354-354: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 546-546: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
[error] 562-562: Unsafe usage of optional chaining. (lint/correctness/noUnsafeOptionalChaining)
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
LanguageTool
README.md
[misspelling] ~55-~55: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ... The URL to request, either a String or a Object that return by [url.parse](https...
[style] ~57-~57: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...ng - Request method, defaults toGET. Could beGET,POST,DELETEorPUT. Al...
[grammar] ~60-~60: The past participle is required after “to be”. (BE_VBP_IN)
Context: ...m_class_stream_readable) - Stream to be pipe to the remote. If set,dataand `cont...
[grammar] ~61-~61: There may an error in the verb form ‘be write’. (MD_BE_NON_VBP)
Context: ...e response stream. Responding data will be write to this stream andcallbackwill be c...
[style] ~63-~63: To form a complete sentence, be sure to include a subject or ‘there’. (MISSING_IT_THERE)
Context: ...tType*** String - Type of request data. Could bejson(Notes: not use `applicat...
[uncategorized] ~63-~63: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ...ation/jsonhere). If it'sjson, will auto setContent-Type: application/json` header...
[style] ~64-~64: To form a complete sentence, be sure to include a subject. (MISSING_IT_THERE)
Context: ...Type*** String - Type of response data. Could betextorjson. If it'stext, th...
[uncategorized] ~64-~64: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ... would be a parsed JSON Object and will auto setAccept: application/jsonheader. Defa...
[grammar] ~67-~67: Possible agreement error. The noun ‘phase’ seems to be countable. (CD_NN)
Context: ... to tell urllib use same timeout on two phase or set them seperately such as `timeout...
[misspelling] ~73-~73: Did you mean “yourself”? Remove the space in between. (YOUR_SELF_TO_YOURSELF)
Context: ...* Function - Format the redirect url by your self. Default isurl.resolve(from, to). ...
[misspelling] ~74-~74: Use ‘every thing’ if you want to emphasize that the things are separate. Usually, you can use “everything”. (EVERYTHING)
Context: ...n - Before request hook, you can change every thing here. - streaming Boolean - let...
[uncategorized] ~76-~76: It appears that a hyphen is missing (if ‘auto’ is not used in the context of ‘cars’). (AUTO_HYPHEN)
Context: ... Acceptgzip, brresponse content and auto decode it, default istrue. - timing...
[misspelling] ~123-~123: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...: 'world', }), });It would make a HTTP request like:bash POST / HTTP...
[uncategorized] ~203-~203: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...uest and response time in ms. -timing: timing object if timing enable. - `sock...
[uncategorized] ~204-~204: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ming object if timing enable. -socket: socket info ## Run test with debug log...
[misspelling] ~240-~240: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’. (EN_A_VS_AN)
Context: ...ad request' }); ``` ## Request through a http proxy export from [undici](https:...
GitHub Check: Node.js / Test (windows-latest, 22)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (windows-latest, 20)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (windows-latest, 18.19.0)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (ubuntu-latest, 22)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (macos-latest, 20)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (ubuntu-latest, 20)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (macos-latest, 22)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
GitHub Check: Node.js / Test (macos-latest, 18.19.0)
src/HttpClient.ts
[failure] 29-29:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
Additional comments not posted (25)
scripts/replace_urllib_version.js (2)
3-4: Use of node: prefix in imports is a best practice.The explicit use of the 'node:' prefix helps in clearly distinguishing Node.js built-in modules from third-party ones. This is a good practice to maintain clarity and avoid potential conflicts.
15-17: Dynamic version replacement logic is robust.The script dynamically replaces the placeholder 'VERSION' with the actual version from
package.json. This ensures that the version is always up-to-date in the distributed files. Logging the changes enhances traceability.test/options.signal.test.ts (2)
3-6: Correct use of node: prefix and file extensions in imports.The updated import statements correctly use the 'node:' prefix for built-in modules and include '.js' extensions for local modules, aligning with Node.js module resolution best practices.
3-6: Good integration of native sleep functionality.Replacing custom sleep implementations with
setTimeoutfrom 'node:timers/promises' standardizes the approach and reduces the codebase's complexity.src/index.ts (2)
1-1: Proper use of named imports for clarity.Changing from default to named imports enhances clarity and reliability, particularly in cases where the module does not actually export a default.
1-1: Well-structured export statements maintain backward compatibility.The export statements are clearly organized, and the use of aliases to maintain compatibility with previous versions of the library is a thoughtful touch.
test/options.timing.test.ts (2)
2-5: Correct use of node: prefix and file extensions in imports.The updated import statements correctly use the 'node:' prefix for built-in modules and include '.js' extensions for local modules, aligning with Node.js module resolution best practices.
2-5: Good integration of native sleep functionality.Replacing custom sleep implementations with
setTimeoutfrom 'node:timers/promises' standardizes the approach and reduces the codebase's complexity.test/options.writeStream.test.ts (3)
6-6: Modernize usage of timers.The import of
setTimeoutfromnode:timers/promisesis a modern approach to handling asynchronous delays in Node.js, aligning with the updates to use native promises.
8-10: Ensure consistency in import paths.The import paths are updated to include the
.jsextension, ensuring compatibility with ES modules in Node.js. This change supports better resolution and avoids ambiguity in module loading.
Line range hint
37-38: Consider platform-specific behavior in tests.The conditional check for Windows to skip certain assertions is a good practice to handle platform-specific differences in behavior, particularly in network and file system operations.
[APROVED]src/utils.ts (1)
6-6: Type annotation added for clarity.The explicit type annotation for
JSONCtlCharsMapasRecord<string, string>improves type safety and clarity, ensuring that the map is used consistently throughout the codebase.test/keep-alive-header.test.ts (2)
2-2: Modernize usage of timers.The import of
setTimeoutfromnode:timers/promisesis a modern approach to handling asynchronous delays in Node.js, aligning with the updates to use native promises.
4-6: Ensure consistency in import paths.The import paths are updated to include the
.jsextension, ensuring compatibility with ES modules in Node.js. This change supports better resolution and avoids ambiguity in module loading.src/diagnosticsChannel.ts (1)
23-23: Refactor and enhance diagnostics channel handling.The updates to the diagnostics channel handling, including the refactoring of the
subscribefunction and improved error handling in socket management, enhance the robustness and maintainability of the code. These changes ensure better error tracking and diagnostics in network operations.Also applies to: 26-29, 43-52, 57-57, 71-71, 101-110, 129-129, 146-163
test/diagnostics_channel.test.ts (2)
3-3: Use ofsetTimeoutfromnode:timers/promisesfor sleep functionality is appropriate.This change aligns with modern Node.js practices by utilizing the promise-based timer functions, which can help avoid callback hell and improve code readability.
5-5: Update to module import paths is correct.The addition of
.jsextensions and the use ofimportfrom specific paths ensure compatibility with ECMAScript modules, which is a best practice in modern JavaScript development.Also applies to: 9-9, 10-10, 11-11
README.md (1)
286-296: Updated Contributors section with dynamic content.Replacing the static list of contributors with a dynamic badge from
contributors-imgis a modern approach to display contributors. It ensures that the list is always up-to-date without manual updates.test/fixtures/server.ts (2)
6-6: Use ofsetTimeoutfromnode:timers/promisesfor sleep functionality is consistent.This change is consistent with the updates in other test files, promoting uniformity across the codebase. Using promise-based timers improves the handling of asynchronous operations.
11-11: Correct import of utility function from a specific path.Updating the import path to include
.jsensures proper resolution of modules when using ECMAScript modules, which is a standard practice for modern Node.js applications.src/HttpClient.ts (5)
14-14: Refactor the import ofpipelineto use a more descriptive name.The import of
pipelineaspipelinePromisemakes it clearer that this is the promise-based version ofpipeline. This is a good practice as it avoids confusion with the callback-based version.
20-20: Use ofsetTimeoutassleepis a good abstraction.Renaming
setTimeouttosleepimproves readability and makes asynchronous delays in the code more intuitive.
22-22: Good update to imports fromundici.Updating imports to use destructuring directly from
undiciis a cleaner approach and improves code readability.
39-39: Consider removing unused imports and functions.It appears that
parseJSON,digestAuthHeader,globalId,performanceTime,isReadableare no longer used in this file. If that's the case, removing these can clean up the code and reduce complexity.
199-199: Potential privacy leak in usingReflect.get.Using
Reflect.getto access properties can lead to unintentional exposure of private properties. Ensure that this does not violate encapsulation principles or leak sensitive information.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
==========================================
- Coverage 98.82% 97.85% -0.97%
==========================================
Files 10 10
Lines 1618 1586 -32
Branches 274 255 -19
==========================================
- Hits 1599 1552 -47
- Misses 19 30 +11
- Partials 0 4 +4 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [4.0.0](v3.25.1...v4.0.0) (2024-06-23) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Updated dynamic badge for contributors in `README.md`. - Enhanced form data handling in `HttpClient`. - **Bug Fixes** - Improved error handling and performance tracking in `Socket` methods. - Adjusted imports for better compatibility and error handling. - **Refactor** - Updated Node.js versions in CI workflow. - Revised `exports` and `scripts` in `package.json` for better consistency. - Simplified `subscribe` function in diagnostics channel. - **Chores** - Removed outdated configurations from `.eslintrc`. - Updated TypeScript compiler options in `tsconfig.json`. - **Documentation** - Linked license section to `contributors-img` in `README.md`. - **Tests** - Refined import paths and added new imports for better error handling in test files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * use undici v6 ([#514](#514)) ([989d228](989d228))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
Summary by CodeRabbit
New Features
README.md.HttpClient.Bug Fixes
Socketmethods.Refactor
exportsandscriptsinpackage.jsonfor better consistency.subscribefunction in diagnostics channel.Chores
.eslintrc.tsconfig.json.Documentation
contributors-imginREADME.md.Tests