Skip to content

feat: add @nkzw/oxlint-config for stricter linting#696

Open
fengmk2 wants to merge 1 commit intonode-modules:masterfrom
fengmk2:feat/oxlint-config
Open

feat: add @nkzw/oxlint-config for stricter linting#696
fengmk2 wants to merge 1 commit intonode-modules:masterfrom
fengmk2:feat/oxlint-config

Conversation

@fengmk2
Copy link
Member

@fengmk2 fengmk2 commented Mar 16, 2026

What

Add @nkzw/oxlint-config for stricter, opinionated linting on top of the existing vp check.

Changes

  • oxlint.config.ts: Extends @nkzw/oxlint-config with sensible overrides for a Node.js library:

    • Disable all React/JSX rules (not a React project)
    • Allow console, any, require() (common in Node.js libraries)
    • Disable perfectionist/sort-* rules (too invasive for existing codebase)
    • Disable no-instanceof (common pattern in Node.js error handling)
    • Ignore test fixtures, esm/cjs/mts test dirs
  • Auto-fixed (~100 issues): prefer-const, numeric-separators-style, array-type, prefer-node-protocol, catch-error-name, text-encoding-identifier-case, curly braces

  • Manually fixed (~20 issues):

    • substring()slice()
    • value.hasOwnProperty()Object.prototype.hasOwnProperty.call()
    • Hex escapes → Unicode escapes
    • Regex literal /\\t/g → string '\t'
    • Short-circuit expressions → if statements
  • npm scripts: Added lint:oxlint and integrated into check

New devDependencies

  • oxlint
  • @nkzw/oxlint-config
  • @nkzw/eslint-plugin
  • eslint-plugin-no-only-tests
  • eslint-plugin-perfectionist
  • eslint-plugin-unused-imports

Summary by CodeRabbit

Release Notes

  • Chores

    • Added Oxlint configuration to enhance code quality checks
    • Integrated new linting tools and ESLint plugins to the development workflow
  • Refactor

    • Improved code consistency with variable naming and type annotations
    • Updated string methods and numeric formatting for better readability
    • Enhanced error handling patterns across the codebase
  • Style

    • Standardized code formatting for numeric literals and method calls
    • Refined syntax consistency in conditional statements

- Add oxlint.config.ts extending @nkzw/oxlint-config with sensible
  overrides for a Node.js library (disable React rules, allow console,
  allow any, etc.)
- Auto-fix: prefer-const, numeric-separators-style, array-type,
  prefer-node-protocol, catch-error-name, text-encoding-identifier-case,
  curly braces
- Manual fix: substring→slice, hasOwnProperty→Object.prototype call,
  hex→unicode escapes, regex string literals, unused expressions
- Add 'lint:oxlint' npm script, integrate into 'check' script
- Disable overly invasive rules (perfectionist/sort-*, no-instanceof)
  that would require massive churn in existing codebase
Copilot AI review requested due to automatic review settings March 16, 2026 07:57
@coderabbitai
Copy link

coderabbitai bot commented Mar 16, 2026

📝 Walkthrough

Walkthrough

Repository-wide refactors standardize type syntax, numeric literals with underscores, and error variable naming. Adds Oxlint configuration and scripts. Core modules adjust minor parsing and string methods, with a small loop break addition in HttpAgent. Tests reflect the same style updates and add guards around optional error causes.

Changes

Cohort / File(s) Summary
Linting & Tooling
oxlint.config.ts, package.json, vite.config.ts
Introduces Oxlint config and lint script, adds devDependencies, updates check scripts, and formats a test timeout literal.
Core Source Updates
src/HttpClient.ts, src/fetch.ts, src/utils.ts, src/HttpAgent.ts, src/FormData.ts, src/IncomingHttpHeaders.ts, src/Request.ts, src/Response.ts, src/diagnosticsChannel.ts
Type syntax switched to Array<...>, parseInt → Number.parseInt, substring → slice, replace → replaceAll, consistent catch variable names; adds a break in address validation loop; safer hasOwnProperty usage.
Examples
examples/*
Renames catch parameters from err → error, formats numbers with underscores, switches var → const where noted, and updates timeout literals.
Build/Script Utility
scripts/replace_urllib_version.js
Changes fs.readFile encoding string from 'utf-8' to 'utf8'.
Tests (Broad Refactors)
test/HttpClient.test.ts, test/diagnostics_channel.test.ts, test/index.test.ts, test/options.*.test.ts, test/esm/index.js, test/cjs/index.js, test/utils.ts, test/urllib.bench.ts
Adopts slice over substring, Number.parseInt over parseInt, underscores in numeric literals, 'utf8' encoding, conditional guards for error.cause, and minor formatting of early returns and string expectations.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • killagu

Poem

I nibble on code like clover at dawn,
Underscores bloom where numbers once yawn.
Errors renamed, they hop in a row,
Arrays stand tall in bracketless show.
Lints whisper softly, “all tidy, all right” —
A rabbit approves, tail tipping in light. 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding @nkzw/oxlint-config for stricter linting, which is the primary objective of the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@socket-security
Copy link

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​nkzw/​eslint-plugin@​2.0.0741006678100
Added@​nkzw/​oxlint-config@​1.0.1721008888100
Addedeslint-plugin-no-only-tests@​3.3.01001009280100
Addedeslint-plugin-unused-imports@​4.4.11001009086100
Addedeslint-plugin-perfectionist@​5.6.010010010092100

View full report

@gemini-code-assist
Copy link

Summary of Changes

Hello, 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 significantly upgrades the project's code quality standards by introducing OXLint with a stricter, opinionated configuration. The integration of this new linter led to a comprehensive refactoring effort, addressing a wide array of stylistic and potential bug-related issues across the codebase, both through automated and manual corrections. This change aims to improve code consistency, maintainability, and catch potential errors earlier in the development cycle.

Highlights

  • OXLint Integration: Integrated @nkzw/oxlint-config to enforce stricter and more opinionated linting rules across the codebase.
  • OXLint Configuration: Configured OXLint with specific overrides tailored for a Node.js library, disabling irrelevant React/JSX rules and allowing common Node.js patterns such as console, any, and require().
  • Automated Code Fixes: Applied numerous automated fixes for common issues, including prefer-const, numeric-separators-style, array-type, prefer-node-protocol, catch-error-name, text-encoding-identifier-case, and curly braces.
  • Manual Code Refinements: Implemented manual code improvements such as replacing substring() with slice(), updating value.hasOwnProperty() calls to Object.prototype.hasOwnProperty.call(), standardizing hex escapes to Unicode escapes, and converting regex literals to string literals for tab characters.
  • NPM Script Updates: Added a new lint:oxlint npm script and integrated it into the existing check and check:fix commands to ensure OXLint runs as part of the standard quality checks.
  • Dependency Management: Added oxlint, @nkzw/oxlint-config, @nkzw/eslint-plugin, eslint-plugin-no-only-tests, eslint-plugin-perfectionist, and eslint-plugin-unused-imports as new development dependencies.
Changelog
  • examples/h2-other-side-closed-exit-0-fetch.cjs
    • Updated error variable names from 'err' to 'error' in catch blocks.
  • examples/h2-other-side-closed-exit-0.cjs
    • Updated error variable names from 'err' to 'error' in catch blocks.
  • examples/httpclient.cjs
    • Applied numeric separator to a timeout value.
    • Updated error variable name from 'err' to 'error' in a catch block.
  • examples/longruning.cjs
    • Applied numeric separator to a loop iteration count.
    • Updated error variable names from 'err' to 'error' in catch blocks.
  • examples/search_github.cjs
    • Replaced 'var' with 'const' for variable declaration.
    • Applied numeric separator to a timeout value.
  • oxlint.config.ts
    • Added a new OXLint configuration file.
    • Extended @nkzw/oxlint-config for base rules.
    • Disabled React/JSX related rules as it's not a React project.
    • Allowed console, any, and require() for Node.js library compatibility.
    • Disabled 'perfectionist/sort-*' rules to avoid invasive changes.
    • Allowed instanceof for common Node.js error handling patterns.
    • Disabled no-undef to permit legitimate global usage in library code.
    • Allowed unicorn/prefer-top-level-await for flexibility.
    • Configured ignorePatterns to exclude 'dist', 'test/fixtures', and various test directories.
  • package.json
    • Added 'lint:oxlint' script to run OXLint.
    • Updated 'check' script to include 'npm run lint:oxlint'.
    • Updated 'check:fix' script to include 'npm run lint:oxlint --fix'.
    • Added 'oxlint' to devDependencies.
    • Added '@nkzw/oxlint-config' to devDependencies.
    • Added '@nkzw/eslint-plugin' to devDependencies.
    • Added 'eslint-plugin-no-only-tests' to devDependencies.
    • Added 'eslint-plugin-perfectionist' to devDependencies.
    • Added 'eslint-plugin-unused-imports' to devDependencies.
  • scripts/replace_urllib_version.js
    • Updated file encoding specification from 'utf-8' to 'utf8' in fs.readFile.
  • src/FormData.ts
    • Refined the NON_ASCII_RE regex to use Unicode escape and added an eslint-disable comment.
    • Replaced replace(/\\/g, '/') with replaceAll('\\', '/') for path normalization.
    • Replaced value.hasOwnProperty('httpVersion') with Object.prototype.hasOwnProperty.call(value, 'httpVersion').
  • src/HttpAgent.ts
    • Updated type annotations from any[] to Array<any>.
    • Added explicit if conditions for early returns in lookup function.
    • Replaced substring with slice for hostname manipulation.
  • src/HttpClient.ts
    • Added an eslint-disable comment for @typescript-eslint/no-empty-object-type.
    • Replaced substring(1) with slice(1) for process.version.
    • Updated type annotations from string[] to Array<string>.
    • Added explicit if conditions for early returns.
    • Replaced parseInt with Number.parseInt for string to number conversions.
    • Updated type annotation for uploadFiles from [] to Array<>.
    • Updated error variable names from 'err' to 'error' in catch blocks.
  • src/IncomingHttpHeaders.ts
    • Updated type annotation for 'set-cookie' header from string[] to Array<string>.
  • src/Request.ts
    • Updated type annotations from number[] to Array<number> for timeout.
    • Updated type annotations from any[] to Array<any> for beforeRequest.
  • src/Response.ts
    • Updated type annotations from string[] to Array<string> for array properties.
  • src/diagnosticsChannel.ts
    • Added explicit if conditions for early returns in several functions.
  • src/fetch.ts
    • Added explicit if conditions for early returns.
    • Updated error variable names from 'e' to 'error' in catch blocks.
    • Replaced parseInt with Number.parseInt for content-length parsing.
  • src/utils.ts
    • Standardized control character string literals to use String.raw.
    • Added an eslint-disable comment for no-control-regex.
    • Replaced substring with slice for string manipulation.
    • Replaced replace(/["']/g, '') with replaceAll(/["']/g, '').
    • Updated global variable checks from typeof global.X === 'undefined' to global.X === undefined.
    • Updated error variable names from 'err' to 'error' in catch blocks.
    • Added explicit if conditions for early returns.
  • test/HttpClient.test.ts
    • Replaced substring with slice in URL string manipulation.
    • Added explicit if conditions for early returns in checkAddress callbacks.
  • test/cjs/index.js
    • Updated assert import to use node:assert for consistency.
  • test/diagnostics_channel.test.ts
    • Added an explicit if condition for an early return.
  • test/esm/index.js
    • Updated assert import to use node:assert for consistency.
  • test/index.test.ts
    • Replaced substring with slice in URL string manipulation within tests.
  • test/keep-alive-header.test.ts
    • Replaced parseInt with Number.parseInt for string to number conversions.
    • Replaced substring with slice in URL string manipulation.
    • Updated error variable names from 'err' to 'error' in catch blocks.
  • test/options.data.test.ts
    • Replaced substring with slice in URL query parsing.
  • test/options.dataType.test.ts
    • Updated error message assertions to use String.raw and match Node.js version specific messages.
  • test/options.digestAuth.test.ts
    • Applied numeric separators to timeout values.
  • test/options.dispatcher.test.ts
    • Applied numeric separator to a timeout value.
  • test/options.files.test.ts
    • Updated file encoding specification from 'utf-8' to 'utf8' in fs.readFile.
  • test/options.fixJSONCtlChars.test.ts
    • Updated tab character replacement to use replaceAll and String.raw.
  • test/options.followRedirect.test.ts
    • Applied numeric separator to a timeout value.
  • test/options.retry.test.ts
    • Replaced parseInt with Number.parseInt in test assertions.
  • test/options.stream.test.ts
    • Updated file encoding specification from 'utf-8' to 'utf8' in readFile.
    • Added explicit if conditions for err.cause checks.
  • test/options.streaming.test.ts
    • Applied numeric separator to a large number assertion.
  • test/options.timeout.test.ts
    • Added explicit if conditions for err.cause checks.
  • test/options.writeStream.test.ts
    • Applied numeric separator to a file size assertion.
    • Updated file encoding specification from 'utf-8' to 'utf8' in readFile.
  • test/urllib.bench.ts
    • Applied numeric separators to timeout values in benchmark configurations.
  • test/urllib.options.allowH2.test.ts
    • Applied numeric separators to timeout values.
  • test/utils.ts
    • Updated type annotation for chunks from Buffer[] to Array<Buffer>.
    • Replaced parseInt with Number.parseInt for Node.js version parsing.
  • vite.config.ts
    • Applied numeric separator to testTimeout configuration.
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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

  1. 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.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces oxlint with the @nkzw/oxlint-config for stricter linting, which is a great step towards improving code quality and consistency. The changes include the new oxlint configuration, updates to package.json for scripts and dependencies, and a large number of automated and manual fixes across the codebase to adhere to the new linting rules. The fixes, such as using Object.prototype.hasOwnProperty.call, replaceAll, and preferring slice over substring, are all definite improvements. I have one suggestion regarding the no-undef rule configuration to make the linting even safer.

'@nkzw/no-instanceof': 'off',

// Library code legitimately uses no-undef globals (Buffer, etc.)
'no-undef': 'off',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Disabling the no-undef rule for the entire project can be risky as it might hide legitimate bugs caused by typos or forgotten imports. A safer approach would be to declare the expected globals where they are used.

While oxlint.config.ts might not support a globals key, oxlint (and ESLint) respects inline global declarations. For files that use Node.js globals like Buffer or process, you could add a comment at the top of the file:

/* globals Buffer, process */

This would allow you to re-enable the no-undef rule (or set it to warn), providing better protection against undefined variables. Please consider this alternative approach for enhanced safety.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR primarily standardizes code style across src/tests/examples (numeric separators, slice()/replaceAll(), Number.parseInt, explicit blocks, node: imports) and introduces a standalone oxlint configuration + execution in the project “check” workflow.

Changes:

  • Add oxlint configuration (oxlint.config.ts) and run it as part of npm run check.
  • Apply lint-driven refactors across tests/examples and internal utilities (string/raw handling, parsing, array typings).
  • Minor improvements to lint suppressions and readability (expanded one-line if/return/continue).

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vite.config.ts Minor test config readability tweak (numeric separator).
test/utils.ts Type/style tweaks (Array<Buffer>, Number.parseInt).
test/urllib.options.allowH2.test.ts Numeric separators for timeouts.
test/urllib.bench.ts Numeric separators for timeouts.
test/options.writeStream.test.ts Numeric separators; normalize utf8 encoding label.
test/options.timeout.test.ts Expand short-circuit assertions into explicit if blocks.
test/options.streaming.test.ts Numeric separators for large byte counts.
test/options.stream.test.ts Normalize utf8 encoding label; explicit if blocks for cause.
test/options.retry.test.ts Use Number.parseInt for header parsing.
test/options.followRedirect.test.ts Numeric separators for timeouts.
test/options.fixJSONCtlChars.test.ts Use replaceAll + String.raw for tab escaping.
test/options.files.test.ts Normalize utf8 encoding label.
test/options.dispatcher.test.ts Numeric separators for timeouts.
test/options.digestAuth.test.ts Numeric separators for timeouts.
test/options.dataType.test.ts Use String.raw to simplify expected escaped error messages.
test/options.data.test.ts Prefer slice() over substring().
test/keep-alive-header.test.ts Prefer slice(), Number.parseInt, and rename errerror.
test/index.test.ts Prefer slice() over substring() for URL origin extraction.
test/HttpClient.test.ts Prefer slice(); expand one-line if returns into blocks.
test/esm/index.js Switch to node:assert import.
test/diagnostics_channel.test.ts Expand one-line if (!handler) return; into block.
test/cjs/index.js Switch to node:assert require.
src/utils.ts Refactors to satisfy lint rules (String.raw, replaceAll, slice, clearer eslint disable).
src/Response.ts Prefer Array<string> type style.
src/Request.ts Prefer Array<number> and Array<any> type style.
src/IncomingHttpHeaders.ts Prefer Array<string> for set-cookie.
src/HttpClient.ts Lint-driven refactors (slice, Number.parseInt, block bodies, typed arrays).
src/HttpAgent.ts Lint-driven refactors (explicit blocks, slice, Array<any>).
src/FormData.ts Lint-driven refactors (replaceAll, safer hasOwnProperty, regex tweak).
src/fetch.ts Lint-driven refactors (continue block, rename eerror, Number.parseInt).
src/diagnosticsChannel.ts Expand one-line returns into blocks for readability/lint compliance.
scripts/replace_urllib_version.js Normalize utf8 encoding label.
package.json Add oxlint + related config deps; run oxlint in check/check:fix.
oxlint.config.ts New oxlint configuration extending @nkzw/oxlint-config.
examples/search_github.cjs Modernize declaration; numeric separator for timeout.
examples/longruning.cjs Numeric separator; rename errerror; error handling style tweaks.
examples/httpclient.cjs Numeric separator for timeout; rename errerror.
examples/h2-other-side-closed-exit-0.cjs Rename errerror and update references.
examples/h2-other-side-closed-exit-0-fetch.cjs Rename errerror and update references.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 70 to +104
@@ -82,6 +83,8 @@
"devDependencies": {
"@arethetypeswrong/cli": "^0.18.2",
"@eggjs/tsconfig": "^2.0.0",
"@nkzw/eslint-plugin": "^2.0.0",
"@nkzw/oxlint-config": "^1.0.1",
"@types/busboy": "^1.5.4",
"@types/mime-types": "^2.1.4",
"@types/node": "^22.19.15",
@@ -94,7 +97,11 @@
"@vitest/coverage-v8": "^4.1.0",
"busboy": "^1.6.0",
"cross-env": "^10.1.0",
"eslint-plugin-no-only-tests": "^3.3.0",
"eslint-plugin-perfectionist": "^5.6.0",
"eslint-plugin-unused-imports": "^4.4.1",
"iconv-lite": "^0.6.3",
"oxlint": "^1.55.0",
Comment on lines +13 to +15
'@typescript-eslint/no-explicit-any': 'off',
// Allow require() for CJS compat
'@typescript-eslint/no-require-imports': 'off',
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (4)
test/options.timeout.test.ts (2)

155-157: Inconsistent guards weaken test assertions.

The if (err.cause) guards at lines 155-157 and 177-179 are inconsistent with the unconditional assertions at lines 41-43 and 65-67 in this same file.

According to src/HttpClient.ts (lines 769-776), timeout errors are always constructed with { cause: err }, so err.cause is guaranteed to be set. These guards allow the test to pass silently if cause is unexpectedly missing, potentially masking regressions.

Consider keeping unconditional assertions for consistency, or if the guards are intentional, apply them uniformly throughout the file.

Also applies to: 177-179

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/options.timeout.test.ts` around lines 155 - 157, Remove the conditional
guards that check `if (err.cause)` in the failing assertions and make the
assertions unconditional so the test fails loudly if cause is missing;
specifically, change the guarded checks that assert
`assert.equal(err.cause.name, 'BodyTimeoutError')` (the blocks currently wrapped
with `if (err.cause) { ... }`) to unconditional assertions, and apply the same
change to the other instance currently guarded (the second block around the same
assertion) so both use direct `err.cause` assertions for consistency with
HttpClient's guaranteed `{ cause: err }` construction.

131-135: Same concern: guard may weaken the BodyTimeoutError assertion.

This guard has the same issue—err.cause should always be set for HttpClientRequestTimeoutError instances created from BodyTimeoutError. The guard allows the test to pass without verifying the expected cause.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/options.timeout.test.ts` around lines 131 - 135, The test currently
weakens verification by conditionally checking err.cause; instead ensure the
HttpClientRequestTimeoutError always has a cause of BodyTimeoutError by removing
the guard and asserting err.cause is defined and its properties match:
assert(err.cause) (or assert.instanceOf if available), then
assert.equal(err.cause.name, 'BodyTimeoutError'),
assert.equal(err.cause.message, 'Body Timeout Error'), and
assert.equal(err.cause.code, 'UND_ERR_BODY_TIMEOUT') so the test fails if the
cause is missing or incorrect (reference symbols: err, err.cause,
HttpClientRequestTimeoutError, BodyTimeoutError).
test/options.stream.test.ts (1)

123-125: Consider whether these guards weaken the test assertions.

Based on the code in src/HttpClient.ts (lines 769-777), HttpClientRequestTimeoutError is always constructed with { cause: err } when wrapping HeadersTimeoutError. This means err.cause should always be defined for these timeout scenarios.

Adding the if (err.cause) guard makes the test silently pass even if cause is unexpectedly missing, which could mask a regression. If the intent is to ensure the cause is set, consider keeping an unconditional assertion or adding an explicit check that cause exists.

Also applies to: 150-152

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/options.stream.test.ts` around lines 123 - 125, The test currently
guards the timeout cause with an if (err.cause) which can hide regressions;
change the assertion to require the cause exists and then assert its type:
ensure the test for HttpClientRequestTimeoutError checks that err.cause is
present (e.g., assert.ok/exists on err.cause) and then unconditionally assert
err.cause.name === 'HeadersTimeoutError' so the test fails if the wrapper stops
including the cause.
package.json (1)

70-72: Please verify direct oxlint usage matches current repo tooling policy.

This introduces direct oxlint invocation and dependency. If the repo still expects linting via Vite+ wrappers only, this should be routed through the Vite+ command path instead.

Based on learnings: "Do not install Vitest, Oxlint, Oxfmt, or tsdown directly; always use Vite+ commands instead".

Also applies to: 104-104

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` around lines 70 - 72, The package.json currently invokes oxlint
directly in the "lint:oxlint", "check", and "check:fix" scripts; replace those
direct calls with the repo's Vite+ wrapper so linting uses the approved tooling
(e.g., change "lint:oxlint": "oxlint" to use the vp wrapper like "lint:oxlint":
"vp lint" or the repo's canonical vp command, and update "check" and "check:fix"
to call that vp-based lint script instead of running oxlint directly), ensuring
all references to "oxlint" are routed through the vp wrapper to comply with the
"do not install oxlint directly" policy.
🤖 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/FormData.ts`:
- Around line 5-6: Replace the control-character escape regex used by
NON_ASCII_RE with a Unicode property escape to satisfy Biome's
noControlCharactersInRegex rule: change the declaration of NON_ASCII_RE
(currently using /[^\u0000-\u007F]/i) to use a Unicode property escape (for
example /[^\p{ASCII}]/u or /\P{ASCII}/u) and remove the "//
eslint-disable-next-line no-control-regex" comment; ensure the 'u' flag is
present and drop the 'i' flag since it's not needed for ASCII checks.

---

Nitpick comments:
In `@package.json`:
- Around line 70-72: The package.json currently invokes oxlint directly in the
"lint:oxlint", "check", and "check:fix" scripts; replace those direct calls with
the repo's Vite+ wrapper so linting uses the approved tooling (e.g., change
"lint:oxlint": "oxlint" to use the vp wrapper like "lint:oxlint": "vp lint" or
the repo's canonical vp command, and update "check" and "check:fix" to call that
vp-based lint script instead of running oxlint directly), ensuring all
references to "oxlint" are routed through the vp wrapper to comply with the "do
not install oxlint directly" policy.

In `@test/options.stream.test.ts`:
- Around line 123-125: The test currently guards the timeout cause with an if
(err.cause) which can hide regressions; change the assertion to require the
cause exists and then assert its type: ensure the test for
HttpClientRequestTimeoutError checks that err.cause is present (e.g.,
assert.ok/exists on err.cause) and then unconditionally assert err.cause.name
=== 'HeadersTimeoutError' so the test fails if the wrapper stops including the
cause.

In `@test/options.timeout.test.ts`:
- Around line 155-157: Remove the conditional guards that check `if (err.cause)`
in the failing assertions and make the assertions unconditional so the test
fails loudly if cause is missing; specifically, change the guarded checks that
assert `assert.equal(err.cause.name, 'BodyTimeoutError')` (the blocks currently
wrapped with `if (err.cause) { ... }`) to unconditional assertions, and apply
the same change to the other instance currently guarded (the second block around
the same assertion) so both use direct `err.cause` assertions for consistency
with HttpClient's guaranteed `{ cause: err }` construction.
- Around line 131-135: The test currently weakens verification by conditionally
checking err.cause; instead ensure the HttpClientRequestTimeoutError always has
a cause of BodyTimeoutError by removing the guard and asserting err.cause is
defined and its properties match: assert(err.cause) (or assert.instanceOf if
available), then assert.equal(err.cause.name, 'BodyTimeoutError'),
assert.equal(err.cause.message, 'Body Timeout Error'), and
assert.equal(err.cause.code, 'UND_ERR_BODY_TIMEOUT') so the test fails if the
cause is missing or incorrect (reference symbols: err, err.cause,
HttpClientRequestTimeoutError, BodyTimeoutError).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b484b49d-cdc3-45c5-8d3d-41c55136c79a

📥 Commits

Reviewing files that changed from the base of the PR and between b936f85 and 727ed6e.

📒 Files selected for processing (39)
  • examples/h2-other-side-closed-exit-0-fetch.cjs
  • examples/h2-other-side-closed-exit-0.cjs
  • examples/httpclient.cjs
  • examples/longruning.cjs
  • examples/search_github.cjs
  • oxlint.config.ts
  • package.json
  • scripts/replace_urllib_version.js
  • src/FormData.ts
  • src/HttpAgent.ts
  • src/HttpClient.ts
  • src/IncomingHttpHeaders.ts
  • src/Request.ts
  • src/Response.ts
  • src/diagnosticsChannel.ts
  • src/fetch.ts
  • src/utils.ts
  • test/HttpClient.test.ts
  • test/cjs/index.js
  • test/diagnostics_channel.test.ts
  • test/esm/index.js
  • test/index.test.ts
  • test/keep-alive-header.test.ts
  • test/options.data.test.ts
  • test/options.dataType.test.ts
  • test/options.digestAuth.test.ts
  • test/options.dispatcher.test.ts
  • test/options.files.test.ts
  • test/options.fixJSONCtlChars.test.ts
  • test/options.followRedirect.test.ts
  • test/options.retry.test.ts
  • test/options.stream.test.ts
  • test/options.streaming.test.ts
  • test/options.timeout.test.ts
  • test/options.writeStream.test.ts
  • test/urllib.bench.ts
  • test/urllib.options.allowH2.test.ts
  • test/utils.ts
  • vite.config.ts

Comment on lines +5 to +6
// eslint-disable-next-line no-control-regex
const NON_ASCII_RE = /[^\u0000-\u007F]/i;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check lint pipeline tools =="
rg -n '"check"|biome|oxlint|eslint' package.json

echo "== Check current regex and suppression in FormData =="
rg -n 'NON_ASCII_RE|no-control-regex|\\u0000|\\p\{ASCII\}' src/FormData.ts

Repository: node-modules/urllib

Length of output: 689


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Search for oxlint/biome configuration files =="
fd -t f '\.(json|js|yaml|yml)$' | grep -i -E '(oxlint|biome)' | head -20

echo "== Check for .oxlintrc or biome.json =="
find . -maxdepth 2 -type f \( -name '.oxlintrc*' -o -name 'biome.json' -o -name 'oxlint.config.*' \) 2>/dev/null

echo "== Check eslintrc configuration =="
find . -maxdepth 2 -type f -name '.eslintrc*' 2>/dev/null | head -5

echo "== View eslint-plugin config to understand rule coverage =="
rg -A 5 '"no-control-regex"' . 2>/dev/null | head -20

Repository: node-modules/urllib

Length of output: 290


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check for biome.json or oxlint config in repo root =="
ls -la | grep -E '(oxlint|biome|eslint)' || echo "No direct matches in root"

echo "== Search package.json for '@nkzw' config hints =="
rg -A 10 '"@nkzw/eslint-plugin"' package.json

echo "== Verify if vp is a linter/checker (likely biome wrapper) =="
rg '"vp"' package.json -A 2 -B 2

echo "== Check src/FormData.ts full context around Line 6 =="
sed -n '1,15p' src/FormData.ts

Repository: node-modules/urllib

Length of output: 763


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Read biome.json =="
cat biome.json

echo ""
echo "== Read oxlint.config.ts =="
cat oxlint.config.ts

echo ""
echo "== Search for 'vp' in package.json =="
rg '"vp"' package.json

Repository: node-modules/urllib

Length of output: 4693


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check full src/FormData.ts context (lines 1-40) =="
sed -n '1,40p' src/FormData.ts

echo ""
echo "== Verify proposed regex syntax support =="
echo "Checking if /u flag and \\p{ASCII} are supported in Node.js TypeScript..."
</script>

Repository: node-modules/urllib

Length of output: 1741


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check tsconfig.json for target ES version =="
cat tsconfig.json | grep -A 5 '"target"' || echo "No explicit target found"

echo ""
echo "== Verify Unicode property escape is standard in modern Node.js =="
echo "Node.js 12+ supports \\p{ASCII} with /u flag"

echo ""
echo "== Verify the proposed regex is functionally equivalent =="
echo "Original: /[^\\u0000-\\u007F]/i (case-insensitive, inverted ASCII range)"
echo "Proposed: /[^\\p{ASCII}]/u (Unicode property, negated ASCII)"
echo "Both should match the same thing: any non-ASCII character"

Repository: node-modules/urllib

Length of output: 768


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== Check package.json for Node.js version requirement =="
rg '"engines"' package.json -A 3

echo ""
echo "== Check if Biome noControlCharactersInRegex rule is mentioned anywhere =="
rg 'noControlCharactersInRegex' . 2>/dev/null | head -10 || echo "No explicit references found"

Repository: node-modules/urllib

Length of output: 305


ESLint suppress comment won't prevent Biome from flagging the control-character regex

Line 6's ESLint disable applies only to ESLint, not Biome. Since noControlCharactersInRegex is enabled in biome.json's recommended rules, the \u0000 escape will still trigger a linting error. Use a Unicode property escape instead:

Proposed change
-// eslint-disable-next-line no-control-regex
-const NON_ASCII_RE = /[^\u0000-\u007F]/i;
+const NON_ASCII_RE = /[^\p{ASCII}]/u;

This regex is functionally equivalent, compatible with the project's ES2022 target and Node.js 18.19.0+ requirement, and will not trigger linting errors.

📝 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.

Suggested change
// eslint-disable-next-line no-control-regex
const NON_ASCII_RE = /[^\u0000-\u007F]/i;
const NON_ASCII_RE = /[^\p{ASCII}]/u;
🧰 Tools
🪛 Biome (2.4.6)

[error] 6-6: Unexpected control character in a regular expression.

(lint/suspicious/noControlCharactersInRegex)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/FormData.ts` around lines 5 - 6, Replace the control-character escape
regex used by NON_ASCII_RE with a Unicode property escape to satisfy Biome's
noControlCharactersInRegex rule: change the declaration of NON_ASCII_RE
(currently using /[^\u0000-\u007F]/i) to use a Unicode property escape (for
example /[^\p{ASCII}]/u or /\P{ASCII}/u) and remove the "//
eslint-disable-next-line no-control-regex" comment; ensure the 'u' flag is
present and drop the 'i' flag since it's not needed for ASCII checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants