Add node and vitest/globals to tsconfig types - #2276
Conversation
|
@arnaud-dezandee Good eye, thanks! However, currently our I'm putting it on the list for post-transition tasks. |
|
@arnaud-dezandee Actually you can give me a hint, if we should disable any globals, then use explicit imports from vitest or use it as a global. However, I would like to avoid adding anything other than nodejs globals to the default settings. Even right now we have several conflicts between nodejs vs WHATWG types. Anyway, I would like to postpone the discussion and decision after the main transition. |
|
@claude should we add vitest globals to the separate tsconfig or use explicit imports? We want:
|
|
Claude finished @dex4er's task in 7m 32s —— View job RecommendationMerge this PR as-is now, enforce requirement 3 with a lint rule rather than with the type system, and switch to explicit imports ( The key constraint: TypeScript's global scope is per-program, not per-file. Any tsconfig whose What the current state actually is
What is broken today is the editor, and badly. tsserver picks the nearest
Also verified: Options against your three requirements
Why not the separate tsconfig. It requires, for all 38 packages plus root: excluding Why explicit imports is the right end state. It is the only option that satisfies all three at the type level, and it is mechanical: 330 test files, 71 already import from Concrete planNow (this PR): merge it. It costs nothing conceptually — it copies the Now, optionally (cheap, gets you requirement 3 where it matters): enforce it in Biome instead of in TypeScript. "linter": { "rules": { "style": { "noRestrictedGlobals": { "level": "error", "options": { "deniedGlobals": {
"describe": "vitest global: only allowed in test files",
"it": "vitest global: only allowed in test files",
"test": "vitest global: only allowed in test files",
"expect": "vitest global: only allowed in test files",
"vi": "vitest global: only allowed in test files",
"beforeEach": "vitest global: only allowed in test files",
"afterEach": "vitest global: only allowed in test files",
"beforeAll": "vitest global: only allowed in test files",
"afterAll": "vitest global: only allowed in test files"
} } } } },
"overrides": [
{ "includes": ["**/*.test.ts", "**/*.test.tsx", "**/__tests__/**", "**/vitest*.setup.*",
"packages/utility-features/test-utils/**"],
"linter": { "rules": { "style": { "noRestrictedGlobals": "off" } } } }
]I ran this over After the React transition: codemod to explicit imports, One note on the PR itselfSetting
No files were changed — this was a question, not an implementation request. |
|
@claude Rerun the recommendation after React transition is made and now we use React v19 |
|
Claude finished @dex4er's task in 8m 21s —— View job Recommendation, re-run against React 19The blocker you named is cleared: catalog now pins Merge this PR as-is now, then do the explicit-imports switch as a follow-up PR. Drop the Biome-stopgap idea from my previous comment — it was only worth the trouble while the migration was blocked, and it no longer is. A separate test-only tsconfig is still the option I would not pick. I ran the migration for real on four packages to make sure this is a recommendation and not a hope. It works, and it has two sharp edges you need to know about before starting. 1. The PR itself: still correct, and now safer than it was under React 18Re-measured on today's tree (
CI is unaffected either way — New under React 19: So the 2. One thing to expect after merging: 6 errors appear in
|
Description of changes:
compilerOptions.typesarray (["node", "vitest/globals"]) to the roottsconfig.jsonand the sharedpackages/infrastructure/typescript/config/base.json.describe,it,expect, etc.) and Node types available to the type checker without a per-file import, so test files can rely on ambient globals.