Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughStandardizes TypeScript imports to type-only where possible, updates multiple package ESLint configs to enable consistent-type-imports and require-extensions rules (and bumps ESLint), converts many vitest.mock calls to dynamic import forms, adds explicit typing to a knip workspace config, and expands Turbo lint inputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
turbo.json (1)
15-15: Re-includetsxinlint.inputsto avoid cache misses.
lintnow ignoressrc/**/*.tsxchanges while other tasks still track them. That can skip lint reruns for TSX edits.♻️ Proposed fix
- "inputs": ["src/**/*.{cts,mts,ts}", "eslint.config.{cjs,mjs,js,json}"] + "inputs": ["src/**/*.{cts,mts,ts,tsx}", "eslint.config.{cjs,mjs,js,json}"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@turbo.json` at line 15, The lint task's "inputs" array in turbo.json currently omits .tsx files causing lint cache misses; update the "inputs" value used for lint (the "inputs" array entry that currently lists "src/**/*.{cts,mts,ts}" and "eslint.config.{cjs,mjs,js,json}") to also include "src/**/*.{tsx,cts,mts,ts}" so TSX changes are tracked and lint reruns when .tsx files change.packages/codegen/eslint.config.mjs (1)
3-23: Consider extracting this ESLint overlay once.This rules/plugin block is now identical in
packages/apollo-core/eslint.config.mjs,packages/apollo-express/eslint.config.mjs, and this file. Pulling it into a small shared helper would keep the next ESLint/plugin tweak single-sourced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/codegen/eslint.config.mjs` around lines 3 - 23, The ESLint rules/plugins block duplicated across packages should be extracted into a shared helper: create a small exported function (e.g., createRequireExtensionsOverlay) that returns the object passed to buildDefaultConfig (including the rules '@typescript-eslint/consistent-type-imports' config and 'require-extensions/require-extensions' and 'require-extensions/require-index') and the plugins map that includes plugin (the imported '@inversifyjs/eslint-plugin-require-extensions'); replace the inline block in packages/codegen/eslint.config.mjs (and the identical blocks in packages/apollo-core/eslint.config.mjs and packages/apollo-express/eslint.config.mjs) with a call to buildDefaultConfig(...createRequireExtensionsOverlay()) or similar, export the helper from a shared location used by these package configs so future changes are single-sourced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/codegen/src/io/actions/writeDefinitions.spec.ts`:
- Around line 3-8: Replace the dynamic import overloads with the
string-specifier overloads and return a mock shaped like the real module: change
vitest.mock(import('node:fs/promises')) to vitest.mock('node:fs/promises') and
change vitest.mock(import('prettier'), () => ({ default: { format: vitest.fn() }
})) to vitest.mock('prettier', () => ({ format: vitest.fn() })), so the mocked
exports match Prettier’s named-export interface and bypass the strict factory
type-checking.
---
Nitpick comments:
In `@packages/codegen/eslint.config.mjs`:
- Around line 3-23: The ESLint rules/plugins block duplicated across packages
should be extracted into a shared helper: create a small exported function
(e.g., createRequireExtensionsOverlay) that returns the object passed to
buildDefaultConfig (including the rules
'@typescript-eslint/consistent-type-imports' config and
'require-extensions/require-extensions' and 'require-extensions/require-index')
and the plugins map that includes plugin (the imported
'@inversifyjs/eslint-plugin-require-extensions'); replace the inline block in
packages/codegen/eslint.config.mjs (and the identical blocks in
packages/apollo-core/eslint.config.mjs and
packages/apollo-express/eslint.config.mjs) with a call to
buildDefaultConfig(...createRequireExtensionsOverlay()) or similar, export the
helper from a shared location used by these package configs so future changes
are single-sourced.
In `@turbo.json`:
- Line 15: The lint task's "inputs" array in turbo.json currently omits .tsx
files causing lint cache misses; update the "inputs" value used for lint (the
"inputs" array entry that currently lists "src/**/*.{cts,mts,ts}" and
"eslint.config.{cjs,mjs,js,json}") to also include "src/**/*.{tsx,cts,mts,ts}"
so TSX changes are tracked and lint reruns when .tsx files change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7157032e-5849-411d-a146-269e791687a1
📒 Files selected for processing (25)
knip.tspackage.jsonpackages/apollo-core/eslint.config.mjspackages/apollo-core/package.jsonpackages/apollo-core/src/apollo/controllers/ApolloServerController.tspackages/apollo-core/src/apollo/models/inversifyApolloProviderServiceIdentifier.tspackages/apollo-core/src/apollo/modules/ApolloServerContainerModule.spec.tspackages/apollo-core/src/apollo/modules/ApolloServerContainerModule.tspackages/apollo-express/eslint.config.mjspackages/apollo-express/package.jsonpackages/apollo-express/src/apollo/controllers/buildApolloServerExpressController.tspackages/apollo-express/src/apollo/modules/ApolloExpressServerContainerModule.spec.tspackages/apollo-express/src/apollo/modules/ApolloExpressServerContainerModule.tspackages/apollo-fastify/package.jsonpackages/apollo-fastify/src/apollo/modules/ApolloFastifyServerContainerModule.spec.tspackages/apollo-subscription-ws/package.jsonpackages/apollo-subscription-ws/src/apollo/modules/ApolloSubscriptionServerContainerModule.spec.tspackages/codegen/eslint.config.mjspackages/codegen/package.jsonpackages/codegen/src/graphql/actions/generateTsModels.tspackages/codegen/src/io/actions/writeDefinitions.spec.tspackages/codegen/src/io/actions/writeDefinitions.tspackages/codegen/src/io/calculations/parseGraphqlFile.spec.tspackages/codegen/src/io/calculations/readSchemas.spec.tsturbo.json
| vitest.mock(import('node:fs/promises')); | ||
| vitest.mock(import('prettier'), () => ({ | ||
| default: { | ||
| format: vitest.fn(), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/codegen/src/io/actions/writeDefinitions.spec.ts | head -20Repository: inversify/graphql
Length of output: 818
🏁 Script executed:
# Check for vitest config
find . -name 'vitest*.config.*' -o -name 'vite.config.*' | head -5Repository: inversify/graphql
Length of output: 278
🏁 Script executed:
# Check package.json for vitest version
cat packages/codegen/package.json | grep -A2 -B2 'vitest'Repository: inversify/graphql
Length of output: 560
🏁 Script executed:
# Look for any TypeScript errors or build output
find . -name 'tsconfig*.json' | head -3Repository: inversify/graphql
Length of output: 182
🌐 Web query:
Vitest 4.1.0 mock API vitest.mock import Promise overload vs string specifier type checking
💡 Result:
In Vitest v4.x (incl. 4.1.0), vi.mock is intentionally typed with two overloads: one that takes a string specifier, and one that takes a dynamic import() promise. (main.vitest.dev)
vi.mock(path: string, factory?)
vi.mock<T>(module: Promise<T>, factory?)Why the Promise (import()) overload exists (type-checking)
Using the Promise overload is mainly a TypeScript trick to make TS actually resolve the module and therefore:
- type-check the specifier (you’ll get a TS error if it can’t be resolved), and
- infer
Tfrom the real module type viatypeof import(...).
Example:
vi.mock(import('./my-module.js'), async (importOriginal) => {
const mod = await importOriginal() // typed as the real module
return { ...mod, foo: vi.fn() }
})With the plain string overload:
vi.mock('./my-module.js', () => ({ foo: vi.fn() }))TypeScript generally cannot validate that './my-module.js' is a real module just because it’s a string argument (it’s not an import), so you lose that “specifier exists” checking benefit.
Important nuance: runtime still mocks “by string”
Even when you use vi.mock(import(...)), Vitest notes that it still operates on a string internally, not a module object. (main.vitest.dev)
Common pitfall: TS path aliases
Vitest explicitly warns that if you use TypeScript paths aliases, the compiler may not resolve types the way you expect for mocking unless you use real relative paths in the import() form (e.g. import('./path/to/module.js') instead of import('@/module')). (main.vitest.dev)
Citations:
- 1: https://main.vitest.dev/api/vi
- 2: https://main.vitest.dev/api/vi
- 3: https://main.vitest.dev/api/vi
- 4: https://main.vitest.dev/api/vi
🏁 Script executed:
# Check Prettier's actual exports
cat packages/codegen/package.json | grep prettierRepository: inversify/graphql
Length of output: 125
🏁 Script executed:
# Look for any build or CI configuration that might show type errors
cat packages/codegen/vitest.config.mjsRepository: inversify/graphql
Length of output: 162
🌐 Web query:
Prettier 3.8 export structure default export format function TypeScript types
💡 Result:
In Prettier 3.8.x, the prettier package is published with conditional exports (Node ESM vs CJS) and bundled TypeScript types:
exports["."]maps to:"default": "./index.mjs"(ESM entry)"require": "./index.cjs"(CommonJS entry)"types": "./index.d.ts"(TypeScript declarations) (app.unpkg.com)
Does Prettier have a default export?
No. The TypeScript declarations do not define a default export; the API is exposed via named exports (and you can also import * as prettier). (app.unpkg.com)
Correct TS import + format type (Prettier 3.8)
format is a named export and returns a Promise<string>:
import { format, type Options } from "prettier";
const out = await format("const x=1", { parser: "babel" } satisfies Options);Type signature (from index.d.ts):
export function format(source: string, options?: Options): Promise<string>;(app.unpkg.com)
Citations:
- 1: https://app.unpkg.com/prettier%403.8.0/files/package.json
- 2: https://app.unpkg.com/prettier%403.8.0/files/index.d.ts
- 3: https://app.unpkg.com/prettier%403.8.0/files/index.d.ts
🏁 Script executed:
# Check if there's a node_modules or type definitions available
find . -path '*/prettier/package.json' -o -path '*/prettier/*/index.d.ts' 2>/dev/null | head -5Repository: inversify/graphql
Length of output: 131
🏁 Script executed:
# Look at how prettier is imported/used in the actual source code
grep -r "from 'prettier'" packages/codegen/src --include="*.ts" -A2 -B2Repository: inversify/graphql
Length of output: 1948
🏁 Script executed:
cat -n packages/codegen/src/io/actions/writeDefinitions.ts | grep -A5 -B5 "prettier"Repository: inversify/graphql
Length of output: 953
🏁 Script executed:
# Check TypeScript config for any special module resolution
cat packages/codegen/tsconfig.jsonRepository: inversify/graphql
Length of output: 344
🏁 Script executed:
# Check if there's a shared TypeScript config with synthetic defaults
cat packages/codegen/tsconfig.json | jq '.compilerOptions | {esModuleInterop, allowSyntheticDefaultImports}' 2>/dev/null || grep -E 'esModuleInterop|allowSyntheticDefaultImports' packages/codegen/tsconfig.jsonRepository: inversify/graphql
Length of output: 130
🏁 Script executed:
# Check the base config that's extended
find . -path "*foundation-typescript-config*" -name "tsconfig*.json" -type f | head -2 | xargs cat 2>/dev/nullRepository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's check how the production code compiles - see if there are any type errors in the source
cat packages/codegen/src/io/actions/writeDefinitions.tsRepository: inversify/graphql
Length of output: 1206
🏁 Script executed:
# Check the foundation TypeScript config repository or see if esModuleInterop is set elsewhere
find . -name "foundation-typescript-config" -type d 2>/dev/null | head -1Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's see if there's a build output or error log
find . -name "tsbuildinfo" -o -name ".tsbuildinfo" 2>/dev/null | head -3Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Check if the test actually fails by looking for CI logs or checking the actual Prettier type definitions
cat packages/codegen/node_modules/.pnpm/prettier*/node_modules/prettier/index.d.ts 2>/dev/null | head -30Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's find node_modules and check Prettier's type definitions
find . -type f -name "index.d.ts" -path "*/prettier/*" 2>/dev/null | head -1 | xargs cat | head -50Repository: inversify/graphql
Length of output: 2001
🏁 Script executed:
# Alternative: check what's in the pnpm store
find . -type d -name "prettier" -path "*/node_modules/*" 2>/dev/null | head -1 | xargs -I {} cat {}/index.d.ts | head -50Repository: inversify/graphql
Length of output: 2001
🏁 Script executed:
# More direct approach - look for the actual Prettier module
ls -la node_modules/.pnpm/ 2>/dev/null | grep prettierRepository: inversify/graphql
Length of output: 460
🏁 Script executed:
# Get more of the Prettier type definitions to see the format export
find . -type f -name "index.d.ts" -path "*/prettier/*" 2>/dev/null | head -1 | xargs cat | grep -A10 "export.*function format"Repository: inversify/graphql
Length of output: 933
🏁 Script executed:
# Try a different approach - search for "format" in Prettier's types
find . -type f -path "*/prettier/*" -name "*.d.ts" 2>/dev/null | head -3 | xargs grep -l "format" | head -1 | xargs cat | tail -100Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's check if there's a default export declaration
find . -type f -path "*/prettier/*" -name "*.d.ts" 2>/dev/null | head -1 | xargs grep "export default\|export ="Repository: inversify/graphql
Length of output: 45
🏁 Script executed:
# Check if there are any build/compile outputs showing TS errors
find . -name "*.tsbuildinfo" -o -name "dist" -o -name "lib" -type d 2>/dev/null | head -5Repository: inversify/graphql
Length of output: 450
🏁 Script executed:
# Let's check the actual Prettier export more carefully - specifically looking for re-exports or default
find . -type f -path "*/prettier/*" -name "*.d.ts" 2>/dev/null | head -1 | xargs cat | tail -150Repository: inversify/graphql
Length of output: 169
🏁 Script executed:
# Check if the repo has TypeScript strict mode configuration
find . -name "tsconfig*.json" -type f -exec grep -l "strict.*true" {} \; 2>/dev/null | head -3Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's get the full extended TypeScript config to see the actual values
cat node_modules/.pnpm/@inversifyjs+foundation-typescript-config*/node_modules/@inversifyjs/foundation-typescript-config/tsconfig.base.esm.json 2>/dev/null | head -50Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Look at the actual Prettier package.json to understand its export structure
cat node_modules/.pnpm/prettier@3.8.1/node_modules/prettier/package.json | jq '.exports' 2>/dev/null || grep -A20 '"exports"' node_modules/.pnpm/prettier@3.8.1/node_modules/prettier/package.jsonRepository: inversify/graphql
Length of output: 4168
🏁 Script executed:
# Check if esModuleInterop or allowSyntheticDefaultImports is set in the extended config
find . -path "*foundation-typescript-config*" -name "*.json" 2>/dev/null | xargs grep -l "esModuleInterop\|allowSyntheticDefaultImports" | head -1 | xargs cat | head -60Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Also check if the current file has any suppression comments or overrides
cat -n packages/codegen/src/io/actions/writeDefinitions.spec.ts | head -15Repository: inversify/graphql
Length of output: 590
🏁 Script executed:
# Check if the foundation config actually sets esModuleInterop or allowSyntheticDefaultImports anywhere
find node_modules -path "*foundation-typescript-config*" -name "*.json" -type f 2>/dev/null | xargs cat 2>/dev/null | grep -E "esModuleInterop|allowSyntheticDefaultImports" | head -5Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's check Prettier's index.mjs to see if it has a default export at runtime
cat node_modules/.pnpm/prettier@3.8.1/node_modules/prettier/index.mjs 2>/dev/null | head -30Repository: inversify/graphql
Length of output: 1561
🏁 Script executed:
# Check if the type definitions re-export a default anywhere
cat node_modules/.pnpm/prettier@3.8.1/node_modules/prettier/index.d.ts 2>/dev/null | tail -50Repository: inversify/graphql
Length of output: 1190
🏁 Script executed:
# Let's try to understand why `import prettier from 'prettier'` works when there's no default export
# This likely indicates esModuleInterop is enabled somewhere
find . -name "*.config.*" -path "*foundation-typescript*" 2>/dev/null | head -5Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Check if there are any build outputs or error logs in the repo
find . -name "package-lock.json" -o -name "pnpm-lock.yaml" -o -name "yarn.lock" 2>/dev/null | head -1 | xargs grep -o "prettier@[^\"]*" | head -1Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's run a direct TypeScript type check on the test file to see if TS2769 appears
cd packages/codegen && npx tsc --noEmit src/io/actions/writeDefinitions.spec.ts 2>&1 | head -20Repository: inversify/graphql
Length of output: 211
🏁 Script executed:
# Let's check if we can run the actual vitest test to see if it currently fails
cd packages/codegen && npm test -- --run src/io/actions/writeDefinitions.spec.ts 2>&1 | head -50Repository: inversify/graphql
Length of output: 211
🏁 Script executed:
# Also let's examine if there's an ESLint or TypeScript config that shows strict settings
cat .eslintrc.json 2>/dev/null || find . -name ".eslintrc*" -o -name "eslint.config.*" 2>/dev/null | head -1 | xargs catRepository: inversify/graphql
Length of output: 644
🏁 Script executed:
# Check the actual root typescript config
cat tsconfig.json 2>/dev/null | head -50Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Let's check if there's a foundational config that might be setting esModuleInterop
find . -path "*/node_modules/@inversifyjs/foundation-typescript-config*" -name "*.json" 2>/dev/null | xargs cat 2>/dev/null | grep -A5 -B5 "esModuleInterop\|allowSyntheticDefaultImports" | head -30Repository: inversify/graphql
Length of output: 43
🏁 Script executed:
# Check if there's a vitest config that might be doing type-checking
cat packages/codegen/vitest.config.mjsRepository: inversify/graphql
Length of output: 162
🏁 Script executed:
# Let's also check how other test files handle prettier mocking if they exist
find packages/codegen -name "*.spec.ts" -type f -exec grep -l "prettier" {} \;Repository: inversify/graphql
Length of output: 117
Use the string-specifier overload for these mocks.
The promise-based vitest.mock(import(...)) overload enforces TypeScript type-checking of the factory return against the real module shape. Prettier 3.8.1 exports only named exports (no default export), so the factory return { default: { format: vitest.fn() } } fails type validation. The string-specifier overload bypasses this check and allows the mock to load.
💡 Suggested change
-vitest.mock(import('node:fs/promises'));
-vitest.mock(import('prettier'), () => ({
+vitest.mock('node:fs/promises');
+vitest.mock('prettier', () => ({
default: {
format: vitest.fn(),
},
}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vitest.mock(import('node:fs/promises')); | |
| vitest.mock(import('prettier'), () => ({ | |
| default: { | |
| format: vitest.fn(), | |
| }, | |
| })); | |
| vitest.mock('node:fs/promises'); | |
| vitest.mock('prettier', () => ({ | |
| default: { | |
| format: vitest.fn(), | |
| }, | |
| })); |
🧰 Tools
🪛 GitHub Actions: build
[error] 4-4: TS2769: No overload matches this call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codegen/src/io/actions/writeDefinitions.spec.ts` around lines 3 - 8,
Replace the dynamic import overloads with the string-specifier overloads and
return a mock shaped like the real module: change
vitest.mock(import('node:fs/promises')) to vitest.mock('node:fs/promises') and
change vitest.mock(import('prettier'), () => ({ default: { format: vitest.fn() }
})) to vitest.mock('prettier', () => ({ format: vitest.fn() })), so the mocked
exports match Prettier’s named-export interface and bypass the strict factory
type-checking.
1 participant
Changed
eslintto v10.Summary by CodeRabbit
Chores
Style / Linting
Tests