feat: add @nkzw/oxlint-config for stricter linting#696
feat: add @nkzw/oxlint-config for stricter linting#696fengmk2 wants to merge 1 commit intonode-modules:masterfrom
Conversation
- 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
📝 WalkthroughWalkthroughRepository-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 labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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
Changelog
Using Gemini Code AssistThe 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
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 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
|
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.
| '@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.
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
oxlintconfiguration (oxlint.config.ts) and run it as part ofnpm 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.
| @@ -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)
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 }, soerr.causeis guaranteed to be set. These guards allow the test to pass silently ifcauseis 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.causeshould always be set forHttpClientRequestTimeoutErrorinstances created fromBodyTimeoutError. 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),HttpClientRequestTimeoutErroris always constructed with{ cause: err }when wrappingHeadersTimeoutError. This meanserr.causeshould always be defined for these timeout scenarios.Adding the
if (err.cause)guard makes the test silently pass even ifcauseis 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 thatcauseexists.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 directoxlintusage matches current repo tooling policy.This introduces direct
oxlintinvocation 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
📒 Files selected for processing (39)
examples/h2-other-side-closed-exit-0-fetch.cjsexamples/h2-other-side-closed-exit-0.cjsexamples/httpclient.cjsexamples/longruning.cjsexamples/search_github.cjsoxlint.config.tspackage.jsonscripts/replace_urllib_version.jssrc/FormData.tssrc/HttpAgent.tssrc/HttpClient.tssrc/IncomingHttpHeaders.tssrc/Request.tssrc/Response.tssrc/diagnosticsChannel.tssrc/fetch.tssrc/utils.tstest/HttpClient.test.tstest/cjs/index.jstest/diagnostics_channel.test.tstest/esm/index.jstest/index.test.tstest/keep-alive-header.test.tstest/options.data.test.tstest/options.dataType.test.tstest/options.digestAuth.test.tstest/options.dispatcher.test.tstest/options.files.test.tstest/options.fixJSONCtlChars.test.tstest/options.followRedirect.test.tstest/options.retry.test.tstest/options.stream.test.tstest/options.streaming.test.tstest/options.timeout.test.tstest/options.writeStream.test.tstest/urllib.bench.tstest/urllib.options.allowH2.test.tstest/utils.tsvite.config.ts
| // eslint-disable-next-line no-control-regex | ||
| const NON_ASCII_RE = /[^\u0000-\u007F]/i; |
There was a problem hiding this comment.
🧩 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; |
🧰 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.
What
Add
@nkzw/oxlint-configfor stricter, opinionated linting on top of the existingvp check.Changes
oxlint.config.ts: Extends@nkzw/oxlint-configwith sensible overrides for a Node.js library:console,any,require()(common in Node.js libraries)perfectionist/sort-*rules (too invasive for existing codebase)no-instanceof(common pattern in Node.js error handling)Auto-fixed (~100 issues):
prefer-const,numeric-separators-style,array-type,prefer-node-protocol,catch-error-name,text-encoding-identifier-case,curlybracesManually fixed (~20 issues):
substring()→slice()value.hasOwnProperty()→Object.prototype.hasOwnProperty.call()/\\t/g→ string'\t'ifstatementsnpm scripts: Added
lint:oxlintand integrated intocheckNew devDependencies
oxlint@nkzw/oxlint-config@nkzw/eslint-plugineslint-plugin-no-only-testseslint-plugin-perfectionisteslint-plugin-unused-importsSummary by CodeRabbit
Release Notes
Chores
Refactor
Style