feat: add @nkzw/oxlint-config for stricter linting - #696
Conversation
Sorry, something went wrong.
- 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
📝 Walkthrough
WalkthroughRepository-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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labelsenhancement Suggested reviewers
Poem🚥 Pre-merge checks | ✅ 2 | ❌ 1 ❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches 🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Sorry, something went wrong.
Summary of ChangesHello, 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
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.
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
|
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| '@nkzw/no-instanceof': 'off', | ||
|
|
||
| // Library code legitimately uses no-undef globals (Buffer, etc.) | ||
| 'no-undef': 'off', |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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 err→error. |
| 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 e→error, 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 err→error; error handling style tweaks. |
| examples/httpclient.cjs | Numeric separator for timeout; rename err→error. |
| examples/h2-other-side-closed-exit-0.cjs | Rename err→error and update references. |
| examples/h2-other-side-closed-exit-0-fetch.cjs | Rename err→error and update references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| @@ -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", | |||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| // Allow require() for CJS compat | ||
| '@typescript-eslint/no-require-imports': 'off', |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)test/options.timeout.test.ts (2)🤖 Prompt for all review comments with AI agentstest/options.stream.test.ts (1)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 AgentsVerify 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 AgentsVerify 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).package.json (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 AgentsVerify 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.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 AgentsVerify 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.
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
📥 CommitsReviewing 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
Sorry, something went wrong.
| // eslint-disable-next-line no-control-regex | ||
| const NON_ASCII_RE = /[^\u0000-\u007F]/i; |
There was a problem hiding this comment.
⚠️ 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.tsRepository: 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 -20Repository: 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.tsRepository: 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.jsonRepository: 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.
| // eslint-disable-next-line no-control-regex | |
| const NON_ASCII_RE = /[^\u0000-\u007F]/i; | |
| const NON_ASCII_RE = /[^\p{ASCII}]/u; |
[error] 6-6: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
🤖 Prompt for AI AgentsVerify 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.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
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:
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):
npm scripts: Added lint:oxlint and integrated into check
New devDependencies
Summary by CodeRabbit
Release Notes
Chores
Refactor
Style