Skip to content

fix(check): exclude test-runner files from app compatibility scans#2596

Open
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-check-cjs-globals
Open

fix(check): exclude test-runner files from app compatibility scans#2596
NathanDrake2406 wants to merge 1 commit into
cloudflare:mainfrom
NathanDrake2406:nathan/fix-check-cjs-globals

Conversation

@NathanDrake2406

Copy link
Copy Markdown
Contributor

Overview

Item Detail
Goal Keep test-runner files out of application compatibility results.
Root cause vinext check recursively treated every JavaScript and TypeScript file as possible app runtime source.
Core change Use an app-compatibility candidate set that excludes test/spec modules and conventional Jest, Playwright, and Vitest config files.
Expected impact Test-only imports and CommonJS globals no longer appear as app migration blockers.

Why

vinext check should report compatibility for files that can contribute to the application bundle. Test modules and test-runner configuration execute under their own runners, so treating them as vinext runtime source creates migration work that does not exist.

This was reproduced in a real Next.js to vinext migration of the movies-ranking app. The report marked CommonJS globals unsupported in exactly these files:

✗  __dirname / __filename (CommonJS globals) — CJS globals unavailable in ESM — use fileURLToPath(import.meta.url) / dirname(...), or import.meta.dirname / import.meta.filename (Node 22+)
   src/app/mobile-layout-alignment.test.ts
   vitest.config.ts

Both files are outside the vinext application runtime. The same checkout completed vinext build, and its targeted Vitest file passed. With this PR's CLI, the app reports:

Overall: 98% compatible (20 supported, 1 partial, 0 issues)
Area Invariant What this PR changes
Compatibility source discovery Only app-runtime candidates should affect app compatibility. Excludes *.test.*, *.spec.*, and conventional Jest, Playwright, and Vitest config files.
Runtime modules Ordinary shared modules must remain visible even when their names contain config. Keeps root site.config.ts and other non-test-runner configs in both scans.
Import checks Test-only next/* imports do not constrain the deployed app. Applies the same candidate boundary to import support.
Convention checks Test-only CJS globals and ViewTransition imports do not constrain the deployed app. Applies the same candidate boundary to per-file convention checks.

What changed

Scenario Before After
__dirname in a Vitest test/config Reported as an unsupported app migration issue. Excluded from application compatibility scanning.
Unsupported next/* import used only by tests Counted against app compatibility. Excluded from application compatibility scanning.
__dirname or unsupported import in site.config.ts Reported. Still reported.
Validation
  • vp test run tests/check.test.ts: 145 tests passed.
  • vp check: all 2,430 files formatted; no warnings, lint errors, or type errors across 1,080 files.
  • vp run vinext#build: package build passed.
  • Patched vinext check against movies-ranking/.worktrees/nathan-vinext-jsonc-install: 98% compatible, 0 issues.
  • bun run build:vinext in that movies-ranking worktree: all five build environments passed.
  • bunx vitest run src/app/mobile-layout-alignment.test.ts: 4 tests passed.
  • Pre-commit full checks, focused tests, and knip passed.
Risk / compatibility
  • No runtime, build-output, or public API changes.
  • Scanner exclusion is basename-based and anchored.
  • Only test/spec modules and known test-runner config names are excluded.
  • Ordinary runtime config modules remain covered by explicit regressions.
Non-goals
  • Changing support for CommonJS globals in application runtime modules.
  • Inferring the complete Vite module graph during the lightweight compatibility check.
  • Excluding every file whose name contains config.

References

Reference Why it matters
#1740 Existing runtime support for CJS path globals in eligible server modules; not the failing boundary here.
#1650 Older middleware/proxy runtime work; also distinct from this scanner false positive.

vinext check scans test modules and test-runner configuration as though they are bundled into the migrated application. Apps using CommonJS globals only in Vitest files therefore receive unsupported migration issues even when the vinext build succeeds.

Separate the application compatibility candidate set from the general recursive file finder. Exclude test and spec modules plus conventional Jest, Playwright, and Vitest config files, while keeping ordinary runtime config modules visible to import and convention checks.
@pkg-pr-new

pkg-pr-new Bot commented Jul 11, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2596
npm i https://pkg.pr.new/create-vinext-app@2596
npm i https://pkg.pr.new/vinext@2596

commit: d323942

@NathanDrake2406 NathanDrake2406 marked this pull request as ready for review July 11, 2026 14:59
@github-actions

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared d323942 against base 8d0a18d using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 2 regressed · 4 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.6 KB 126.6 KB ⚫ -0.0%
Client entry size (gzip) vinext 120.6 KB 120.6 KB ⚫ -0.0%
Dev server cold start vinext 2.75 s 2.84 s 🔴 +3.0%
Production build time vinext 3.00 s 3.05 s 🔴 +1.7%
RSC entry closure size (gzip) vinext 98.6 KB 98.6 KB ⚫ 0.0%
Server bundle size (gzip) vinext 165.3 KB 165.3 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

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.

1 participant