chore(lint): Rule adjustments and fix warnings#19612
chore(lint): Rule adjustments and fix warnings#19612
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
logaretm
force-pushed
the
awad/oxlint-rules-cleanup
branch
2 times, most recently
from
e9ad91b to
616ec0f
Compare
logaretm
changed the title
logaretm
force-pushed
the
awad/oxlint-rules-cleanup
branch
from
582de79 to
9dfdd14
Compare
There was a problem hiding this comment.
Pull request overview
This PR adjusts the repo’s oxlint configuration and applies a broad set of small code edits to reduce lint warnings (mostly optional chaining simplifications, unused param renames, and inline oxlint suppressions), aiming to restore stricter lint coverage.
Changes:
- Updated
.oxlintrc.json/dev-packages/.oxlintrc.jsonrule configuration (re-enabling certain rules, tuningno-unused-vars, and scoping rule overrides for tests/dev-packages). - Performed mechanical code cleanups across packages (remove redundant
| undefinedfrom optional params, prefix unused catch params with_, optional-chaining rewrites). - Added targeted
oxlint-disable(-next-line)comments where rules produce false positives or are intentionally violated.
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sveltekit/test/server-common/handle.test.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/sveltekit/src/server-common/handle.ts | Added oxlint suppression for optional-chain preference in a narrowing pattern. |
| packages/sveltekit/src/index.types.ts | Simplified optional param type (timeout?: number). |
| packages/svelte/test/components/Dummy.svelte | Added oxlint suppression for an intentionally “assigned by framework” prop. |
| packages/solidstart/src/index.types.ts | Simplified optional param type (timeout?: number). |
| packages/replay-worker/test/unit/Compressor.test.ts | Small test cleanup (remove void from throwing expectation wrapper). |
| packages/replay-internal/src/util/handleRecordingEmit.ts | Optional chaining simplification around replay.session. |
| packages/replay-internal/src/replay.ts | Optional chaining simplification around this.session. |
| packages/replay-internal/src/coreHandlers/util/addNetworkBreadcrumb.ts | Added oxlint suppression for floating-promises in fire-and-forget span creation. |
| packages/replay-internal/src/coreHandlers/handleHistory.ts | Added oxlint suppression for floating-promises in fire-and-forget span creation. |
| packages/replay-internal/src/coreHandlers/handleAfterSendEvent.ts | Optional chaining simplification for event.tags. |
| packages/remix/src/utils/utils.ts | Simplified optional param type (formDataKeys?: ...). |
| packages/remix/src/client/remixRouteParameterization.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/remix/src/client/performance.tsx | Optional chaining for safe last-match access. |
| packages/react/src/reactrouter-compat-utils/route-manifest.ts | Optional chaining simplification for manifest length check. |
| packages/react/src/reactrouter-compat-utils/instrumentation.tsx | Added oxlint suppression for promise.finally(...) floating promise. |
| packages/react/src/hoist-non-react-statics.ts | Optional chaining + unused catch param prefix. |
| packages/opentelemetry/src/utils/parseSpanDescription.ts | Added oxlint suppression for array sort comparator rule. |
| packages/nuxt/src/runtime/utils.ts | Added oxlint suppression for unsafe member access. |
| packages/nuxt/src/runtime/plugins/storage.server.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/node/src/integrations/tracing/openai/instrumentation.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/node/src/integrations/tracing/graphql.ts | Added oxlint suppression for array sort comparator rule. |
| packages/node/src/integrations/tracing/anthropic-ai/instrumentation.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/node-core/src/utils/detection.ts | Added oxlint suppression for prefer-optional-chain in typeof module guard. |
| packages/node-core/src/utils/captureRequestBody.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/node-core/src/transports/http.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/node-core/src/sdk/client.ts | Simplified optional param type (timeout?: number). |
| packages/node-core/src/light/client.ts | Simplified optional param type (timeout?: number). |
| packages/node-core/src/integrations/local-variables/worker.ts | Added oxlint suppression for prefer-optional-chain around null/undefined checks. |
| packages/node-core/src/integrations/local-variables/local-variables-sync.ts | Simplified optional param type + added oxlint suppression for prefer-optional-chain around null/undefined checks. |
| packages/nextjs/src/common/devErrorSymbolicationEventProcessor.ts | Added oxlint suppression for prefer-optional-chain in multi-guard narrowing logic. |
| packages/nextjs/src/client/clientNormalizationIntegration.ts | Switched suppression comment to oxlint for global access guard. |
| packages/google-cloud-serverless/test/gcpfunction/http.test.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/deno/src/utils/streaming.ts | Prefix unused catch param + added oxlint suppression for reader.closed.finally(...). |
| packages/deno/src/client.ts | Simplified optional param type (timeout?: number). |
| packages/core/test/lib/utils/object.test.ts | Added oxlint suppression for intentionally assigned-by-eval variable. |
| packages/core/test/lib/transports/base.test.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/core/src/utils/url.ts | Simplified optional param types for URL parsing helpers. |
| packages/core/src/utils/prepareEvent.ts | Added oxlint suppression for prefer-optional-chain around internal exception marker check. |
| packages/core/src/utils/hasSpansEnabled.ts | Simplified optional param type for options. |
| packages/core/src/utils/exports.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/core/src/utils/debug-logger.ts | Removed info helper (logger interface now exposes log/warn/error). |
| packages/core/src/types-hoist/polymorphics.ts | Simplified redundant type intersections. |
| packages/core/src/tracing/vercel-ai/index.ts | Optional chaining simplification. |
| packages/core/src/tracing/sentrySpan.ts | Simplified optional param type for recordException. |
| packages/core/src/tracing/sentryNonRecordingSpan.ts | Simplified optional param type for recordException. |
| packages/core/src/tracing/openai/utils.ts | Added oxlint suppression for prefer-optional-chain in object/type guard. |
| packages/core/src/tracing/google-genai/index.ts | Added oxlint suppression for prefer-optional-chain in in-operator guard. |
| packages/core/src/integrations/mcp-server/transport.ts | Added oxlint suppression for prefer-optional-chain in explicit null/undefined checks. |
| packages/core/src/integrations/mcp-server/handlers.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/core/src/client.ts | Simplified optional hint param types + added oxlint suppression for prefer-optional-chain around internal exception marker check. |
| packages/cloudflare/test/workflow.test.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/cloudflare/src/request.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/bun/scripts/install-bun.js | Prefix unused callback param to satisfy unused-var conventions. |
| packages/browser/src/tracing/browserTracingIntegration.ts | Optional chaining simplification for supportedEntryTypes. |
| packages/browser/src/stack-parsers.ts | Optional chaining simplification for eval detection. |
| packages/browser/src/profiling/utils.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/browser/src/eventbuilder.ts | Added oxlint suppression for prefer-optional-chain in typeof WebAssembly guard. |
| packages/browser-utils/src/metrics/web-vitals/lib/initUnique.ts | Prefix unused catch param to satisfy unused-var conventions. |
| packages/browser-utils/src/metrics/browserMetrics.ts | Type union tweak related to navigation timing end event names. |
| packages/astro/src/server/middleware.ts | Replaced eslint suppression with oxlint suppression for unsafe member access (and added one more inline). |
| packages/astro/src/integration/snippets.ts | Temporarily disabled prefer-optional-chain for a block generating snippet logic. |
| packages/astro/src/index.types.ts | Simplified optional param type (timeout?: number). |
| packages/angular/src/zone.ts | Updated suppression comment to include prefer-optional-chain (but currently via eslint directive). |
| packages/angular/src/errorhandler.ts | Added oxlint suppressions for prefer-optional-chain in narrowing patterns. |
| dev-packages/size-limit-gh-action/index.mjs | Prefix unused catch param to satisfy unused-var conventions. |
| dev-packages/rollup-utils/plugins/npmPlugins.mjs | Removed unused fs/path imports. |
| dev-packages/node-overhead-gh-action/index.mjs | Prefix unused catch param to satisfy unused-var conventions. |
| dev-packages/node-integration-tests/suites/tracing/google-genai/scenario.mjs | Prefix unused catch param to satisfy unused-var conventions. |
| dev-packages/browser-integration-tests/utils/replayHelpers.ts | Prefix unused catch param to satisfy unused-var conventions. |
| dev-packages/.oxlintrc.json | Turned off additional TS rules for dev-packages. |
| .oxlintrc.json | Updated plugin/rule configuration and rule severities/overrides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "plugins": ["typescript", "import", "jsdoc", "vitest"], | ||
| "jsPlugins": [ |
There was a problem hiding this comment.
.oxlintrc.json removes the jest plugin from plugins, but the config still enables multiple jest/* rules (e.g. jest/no-focused-tests, jest/no-disabled-tests, and several jest/* rules turned off later). With oxlint running in CI, this will typically fail config validation or leave these rules ineffective. Either re-add the jest plugin, or migrate these rules to the vitest/* equivalents and remove the jest/* rules entirely.
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access typescript/prefer-optional-chain | ||
| const isNgZoneEnabled = typeof Zone !== 'undefined' && Zone.root?.run; |
There was a problem hiding this comment.
This is an eslint-disable-next-line directive, but the rule you’re trying to suppress (typescript/prefer-optional-chain) is an oxlint rule name. Since the repo lint script runs oxlint, this comment won’t suppress the oxlint error. Use an oxlint-disable-next-line typescript/prefer-optional-chain comment here instead (keeping the typeof Zone !== 'undefined' check, since optional chaining can’t replace it without risking a ReferenceError).
| // oxlint-disable-next-line typescript/no-floating-promises | ||
| promise.finally(() => { | ||
| const currentPromises = pendingLazyRouteLoads.get(span); |
There was a problem hiding this comment.
Instead of disabling typescript/no-floating-promises here, it would be clearer to explicitly mark the returned promise as intentionally unhandled (the rule is configured with ignoreVoid: true in .oxlintrc.json). Consider prefixing this with void so the intent is explicit and you don’t need a disable comment.
| replay.addUpdate(() => { | ||
| // oxlint-disable-next-line typescript/no-floating-promises | ||
| createPerformanceSpans(replay, [result]); | ||
| // Returning false to flush | ||
| return false; |
There was a problem hiding this comment.
createPerformanceSpans returns an array of promises (Promise<...>[]), so calling it without handling the results is what triggers typescript/no-floating-promises. Rather than suppressing the rule, consider explicitly handling/ignoring the promises (e.g. aggregate and intentionally drop with void), which makes the fire-and-forget behavior clearer and avoids blanket suppression on this line.
| replay.addUpdate(() => { | ||
| // oxlint-disable-next-line typescript/no-floating-promises | ||
| createPerformanceSpans(replay, [result]); | ||
| // Returning true will cause `addUpdate` to not flush |
There was a problem hiding this comment.
Same as in handleHistorySpanListener: createPerformanceSpans returns an array of promises, so the current call is effectively fire-and-forget. Instead of disabling typescript/no-floating-promises, consider explicitly aggregating/handling the promises (or intentionally ignoring them with void) so it’s clear these async operations are intentionally not awaited.
| // oxlint-disable-next-line typescript/no-floating-promises | ||
| reader.closed.finally(() => onDone()); | ||
| return new ReadableStream({ |
There was a problem hiding this comment.
Rather than disabling typescript/no-floating-promises here, consider explicitly marking this as intentionally unhandled (e.g. prefix with void). This keeps the rule enabled while still documenting that reader.closed.finally(...) is fire-and-forget.
.oxlintrc.json
Outdated
| "typescript/prefer-optional-chain": ["error"], | ||
| "typescript/no-redundant-type-constituents": "off", | ||
| "typescript/restrict-template-expressions": "off", | ||
| "typescript/await-thenable": "off", |
There was a problem hiding this comment.
awaiton non-Promises is valid JS — it's a useful pattern for uniformly handlingT | Promise<T>without branching. Not a bug.
FWIW the rule should account for this. At least the typescript-eslint one will allow awaiting a T | Promise<T>. I haven't checked the oxlint one.
There was a problem hiding this comment.
@JoshuaKGoldberg I closed the PR by mistake with claude's help 🤦♂️
I disabled it since it has some false positives, but the production ones look genuine so I will fix them instead. thanks!
Most await-thenable violations are in tests or are false positives where code intentionally handles T | Promise<T> uniformly. Keep as warn for production source to catch genuinely unnecessary awaits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Remove unnecessary await on serializeEnvelope (returns sync value). Suppress remaining await-thenable in cron callbacks and server action instrumentation where callbacks may be async at runtime despite types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Removed
undefinedfrom optional params | 19 |param?: T | undefined → param?: T— the ? already implies undefined
We explicitly added undefined in probably most of these cases because there's the exactOptionalPropertyTypes rule which some of our users have enabled. I'm wondering why oxlint is flagging these in the first place but would advocate to rather disable the respective rule. WDYT?
| // oxlint-disable-next-line typescript/prefer-optional-chain | ||
| if (message.id !== null && message.id !== undefined) { |
There was a problem hiding this comment.
super-l: we could simplify this further, right? Would this allow us to get rid of the optional chain disable rule?
| // oxlint-disable-next-line typescript/prefer-optional-chain | |
| if (message.id !== null && message.id !== undefined) { | |
| // oxlint-disable-next-line typescript/prefer-optional-chain | |
| if (message.id != null) { |
| // oxlint-disable-next-line typescript/prefer-optional-chain | ||
| 'config' in params && | ||
| params.config && | ||
| typeof params.config === 'object' && | ||
| 'systemInstruction' in params.config && | ||
| params.config.systemInstruction |
There was a problem hiding this comment.
l: hmm so on first glance it looks like we could indeed simplify this but I'm wondering if TS would complain because of how params is typed? Or could it be that prefer-optional-chain is buggy and doesn't know how to handle unknown?
EDIT: seeing how it flags other cases like #19612 (comment), I think this rule is a bit buggy, no?
| * @internal | ||
| */ | ||
| public recordException(_exception: unknown, _time?: number | undefined): void { | ||
| public recordException(_exception: unknown, _time?: number): void { |
There was a problem hiding this comment.
h: see my other comment above regarding exactOptionalPropertyTypes. Unless I'm missing something (?)
| function info(...args: Parameters<typeof console.info>): void { | ||
| _maybeLog('info', ...args); | ||
| } |
| // oxlint-disable-next-line typescript/prefer-optional-chain | ||
| const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true; |
There was a problem hiding this comment.
super-l: since we're type-casting anyway already, we could simpify this to
| // oxlint-disable-next-line typescript/prefer-optional-chain | |
| const isInternalException = hint.data && (hint.data as { __sentry__: boolean }).__sentry__ === true; | |
| // oxlint-disable-next-line typescript/prefer-optional-chain | |
| const isInternalException = (hint.data as { __sentry__?: boolean })?.__sentry__ === true; |
which might cut down bundle size ever so slightly? Again, feel free to disregard
This is a follow up PR that cleans up our configuration and reverts the downgrade to warning for some of the rules we use. This brings us to a similar level of coverage with eslint.
Some rules have sensitivity issue, especially when it comes to optional chaining and types so we will still have a lot of warnings.
Summary of Changes
Config changes (
.oxlintrc.json)Globally disabled (TS files)
no-redundant-type-constituents'literal' | stringfor autocomplete hints, andunknown | Xpatterns are common throughout the codebase. Low bug-catching value.restrict-template-expressionsunknownvalues in template strings. Would requireString()wrappers everywhere for minimal safety gain — the SDK handles these at runtime.await-thenableawaiton non-Promises is valid JS — it's a useful pattern for uniformly handlingT | Promise<T>without branching. Not a bug.no-base-to-string[object Object]in strings is a real issue, but not blocking CI while we clean up the 22 remaining source violations.Disabled in tests + dev-packages only
no-misused-spreadrequire-array-sort-compare.sort()without comparator is fine for strings.no-base-to-stringConfigured
no-unused-vars_prefix ignore patterns (argsIgnorePattern,varsIgnorePattern,caughtErrorsIgnorePattern). Standard convention — unused catch params/args prefixed with_are intentional.Dev-packages config (
dev-packages/.oxlintrc.json)Added
require-array-sort-compare,no-misused-spread, andno-base-to-stringas off — these rules aren't worth enforcing in test infrastructure.Code fixes
| undefinedfrom optional paramsparam?: T | undefined→param?: T— the?already impliesundefined_catch (error)→catch (_error)— follows the_convention for intentionally unused variables(error, version)→(error, _version)inbun/scripts/install-bun.jsResult
373 warnings → 31 (22 of which are the intentional
no-base-to-stringwarnings we kept visible).Closes #19718 (added automatically)