fix: constrain root tsconfig project scope#1273
Conversation
The root TypeScript project previously relied on TypeScript's implicit include set, which pulled independent workspace app files and nested package build output into root tooling. That made standard tsc checks report app-local alias and Workers type errors through the vinext package project. Add an explicit include list for root-owned config files, vinext source, and root tests. Cover the boundary with a regression test that asserts independent apps, generated package output, and local worktree directories stay outside the parsed root project.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| "tests/**/*.ts", | ||
| "tests/**/*.tsx" | ||
| ], | ||
| "exclude": ["node_modules", "dist", "fixtures", "tests/fixtures", "benchmarks", "examples"] |
There was a problem hiding this comment.
Nit: With an explicit include, most of the exclude entries are now redundant — node_modules, dist, fixtures, benchmarks, and examples are already outside the include globs. The only entry that still does work is tests/fixtures (since tests/**/*.ts would otherwise pull in fixture files).
Not a blocker — the extra excludes are harmless as defense-in-depth — but if you want to tidy this up later, the exclude could be trimmed to just ["tests/fixtures"].
| expect(fileNames.some((fileName) => fileName.startsWith("apps/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith("packages/vinext/dist/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith(".claude/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith(".worktrees/"))).toBe(false); |
There was a problem hiding this comment.
The PR description mentions .refs/ as a directory that existed and caused issues during formatting. It's gitignored (or at least not committed), but if someone has it locally, include already keeps it out. If you wanted to be thorough, you could add a .refs/ assertion here too — but it's a truly optional polish since the directory isn't in the repo.
|
Review submitted. Here's a summary: Approved with two minor, non-blocking observations:
The change is correct: the |
| expect(fileNames.some((fileName) => fileName.startsWith("apps/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith("packages/vinext/dist/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith(".claude/"))).toBe(false); | ||
| expect(fileNames.some((fileName) => fileName.startsWith(".worktrees/"))).toBe(false); |
There was a problem hiding this comment.
dist entries should already be excluded. you can add .claude, .worktrees, .nextjs-ref, etc. to exclude if you want? otherwise, i'd rather it includes everything in the repo and excludes stuff that isn't part of the repo. there is also no need for a test file for this.
2 participants
Overview
tsconfig.jsoninclude list for root config files,packages/vinext/src, and root tests.tests/root-tsconfig.test.tsto parse the root config and assert independent app files, package build output, and local worktree dirs stay outside the root project.apps/webalias or Cloudflare type errors through the root vinext package project.Why
The root quality config should describe the vinext package workspace boundary, not every TypeScript file that happens to exist under the checkout. Without an explicit include list, TypeScript falls back to a repo-wide default and pulls in independent app files plus generated declaration output.
What changed
tsconfig.jsonrelied on implicit file inclusion plus exclusions.tsconfig.jsonopts in to root TS files, vinext source, and tests.tsc --showConfig --project tsconfig.jsonincludedapps/web/**andpackages/vinext/dist/**/*.d.ts.0files underapps/,packages/vinext/dist/,.claude/, and.worktrees/.Validation
vp test run tests/root-tsconfig.test.tsvp check tests/root-tsconfig.test.tsvp fmt --check tsconfig.json tests/root-tsconfig.test.tsnpx tsc --showConfig --project tsconfig.json --pretty falseinspected with a small Node check, confirming0absorbed files underapps/,packages/vinext/dist/,.claude/, and.worktrees/npx tsc --noEmit --project tsconfig.json --pretty falsestill fails on pre-existingtests/app-fallback-renderer.test.tsTS2322 fixture typing errors, but the reportedapps/webalias and Cloudflare type errors are gone.Notes
The local commit hook was bypassed because it attempted to format checked-out reference repositories under
.refs/and failed on intentional syntax-error fixtures there. The targeted repo checks above were run after that failure.