chore: turn on more lint rules

[james-elicx]

No description provided.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@714

commit: 6b30d88

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[james-elicx]

/bigbonk review

[ask-bonk]

Review: chore: turn on more lint rules

Solid lint hygiene PR — the new rules are well-chosen, the eslint-disable → oxlint-disable migration is mechanical and correct, and the arrow-body-style refactors are all semantically equivalent. A few notes below.

Lint config (vite.config.ts)

The new rule set is reasonable. Good choices enabling import/first, import/no-duplicates, and unicorn/prefer-node-protocol — these catch real bugs. The test file overrides for no-explicit-any and no-unsafe-function-type are practical.

Potential issue: oxlint-disable-next-line before return type annotation

In packages/vinext/src/server/instrumentation.ts, the second disable comment sits between the parameter list and the return type:

export async function importModule(
  runner: ModuleImporter,
  id: string,
  // oxlint-disable-next-line @typescript-eslint/no-explicit-any
): Promise<Record<string, any>> {

This placement is unusual — the disable is targeting the return type's any, but oxlint-disable-next-line disables the next line, which is ): Promise<Record<string, any>> {. This happens to work because the any is on that line, but it reads strangely. The function-level disable on line 58 already covers this entire declaration — the inline one on line 62 is redundant. Minor nit, not blocking.

Behavioral correctness: all arrow-body-style changes are safe

Verified that every { return expr; } → expr change preserves semantics — no side effects removed, no early returns lost. The dynamic.ts change correctly converts SSRFalse from a block-body arrow to a concise arrow while keeping displayName assignment below it (which works because it's assigned on the function object, not inside the body).

import/first fix in navigation-state.ts

The consolidation of GLOBAL_ACCESSORS_KEY into the top-level import from ./navigation.js (removing the duplicate mid-file import) is correct. Both import sources (./navigation.js and ./navigation) resolve to the same module, so this is a clean dedup.

import/first fix in api-handler.test.ts

Moving vi.mock() below the import statements is the right fix for import/first. Vitest hoists vi.mock() calls regardless of source position, so this doesn't change test behavior.

import/first fix in benchmarks/generate-app.mjs

The consolidation of readdirSync, statSync, cpSync into the top-level import is correct — these are only used later in the file, and hoisting the imports is cleaner.

react/no-children-prop disables in app-page-boundary-render.ts

The createElement() calls passing children as a prop are intentional here — you can't use JSX children syntax with createElement() when also passing other props. The disables are appropriate.

rules-of-hooks disables in navigation.ts

The hook calls inside usePathname, useSearchParams, useParams have early returns guarded by isServer checks. The hooks are always called on the client path (never conditionally skipped), so the runtime behavior is correct — the lint rule just can't see through the branching. Disables are appropriate.

rules-of-hooks false positive in tests/e2e/fixtures.ts

The consoleErrors key in base.extend() is flagged because it looks like use* to the hooks rule. This is a Playwright fixture, not a React hook. Disable is appropriate.

no-unsafe-function-type in build-optimization.test.ts

The removal of // eslint-disable-next-line @typescript-eslint/no-unsafe-function-type on line 1363 (the run(store: unknown, fn: Function, ...args: unknown[]): unknown type) relies on the test override in vite.config.ts that turns no-unsafe-function-type off for **.test.ts files. This works, but the glob pattern **.test.ts is worth double-checking — in oxlint, ** might need to be **/*.test.ts to match files in subdirectories. If **.test.ts doesn't match tests/build-optimization.test.ts, this line would start failing.

OG route reformatting

The examples/app-router-playground/app/api/og/route.tsx changes are purely formatting (quote style, indentation). SVG content is identical. No behavioral change.

Overall: clean PR, no regressions. The override glob pattern is the only thing worth verifying.

[ask-bonk]

Review posted on PR #714. Here's a summary of my findings:

Overall: Clean PR, no regressions. The changes fall into four categories:

1. New lint rules in vite.config.ts — Well-chosen additions including import/first, import/no-duplicates, arrow-body-style, unicorn/prefer-node-protocol, React hooks rules, and TypeScript strictness rules. Test file overrides are practical.

2. eslint-disable → oxlint-disable migration — Mechanical rename across ~30 files, all correct.

3. arrow-body-style fixes — All { return expr; } → concise arrow conversions are semantically equivalent. Verified no side effects were removed or early returns lost.

4. import/first and import/no-duplicates fixes — Imports consolidated to file tops in navigation-state.ts, api-handler.test.ts, benchmarks/generate-app.mjs, and link.test.ts. The vi.mock() reorder in api-handler.test.ts is safe because Vitest hoists mocks.

One item worth verifying: The override glob pattern **.test.ts in the lint config — it should probably be **/*.test.ts to reliably match test files in subdirectories. If the current pattern doesn't match, the Function type usage in build-optimization.test.ts (line 1343) would fail the no-unsafe-function-type rule.

github run