feat: infer types for serverFetch#4399
Conversation
|
@p-dubovitsky is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughRefactors ChangesserverFetch typed export and fetch type fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
commit: |
![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 (1)
test/unit/server-fetch-types.test-d.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover uppercase methods with a positive assertion.
This suite exercises lowercase
"post"and an invalid uppercase"GET", but the newLowercase<...>path is what makes uppercase"POST"work for known routes. Add one passing uppercase case so that compatibility surface stays pinned.Suggested addition
const upload = await serverFetch("/api/upload", { method: "post" }); expectTypeOf(await upload.json()).toEqualTypeOf<string>(); + + const uploadUpper = await serverFetch("/api/upload", { method: "POST" }); + expectTypeOf(await uploadUpper.json()).toEqualTypeOf<string>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/server-fetch-types.test-d.ts` around lines 13 - 14, Add a positive type assertion in server-fetch-types.test-d.ts that exercises an uppercase known method, not just lowercase "post" and invalid "GET"; update the serverFetch test case to call a known route like /api/upload with method "POST" and assert the returned JSON type, so the Lowercase<...> behavior stays covered in the serverFetch test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vitest.config.ts`:
- Around line 13-15: The Vitest typecheck config is hiding diagnostics from
imported source files because `typecheck.ignoreSourceErrors` is enabled, so
source regressions can slip through even when the test d.ts files pass. Update
`vitest.config.ts` to stop suppressing source-file errors in the `typecheck`
block, and if you still need separate source validation, move that
responsibility to a dedicated source typecheck step rather than `vitest
--typecheck.only`.
---
Nitpick comments:
In `@test/unit/server-fetch-types.test-d.ts`:
- Around line 13-14: Add a positive type assertion in
server-fetch-types.test-d.ts that exercises an uppercase known method, not just
lowercase "post" and invalid "GET"; update the serverFetch test case to call a
known route like /api/upload with method "POST" and assert the returned JSON
type, so the Lowercase<...> behavior stays covered in the serverFetch test
suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12c2b568-f63b-49b1-91d2-19e0f483a1e9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
docs/4.examples/server-fetch.mdexamples/server-fetch/README.mdexamples/server-fetch/routes/index.tsexamples/server-fetch/tsconfig.jsonpackage.jsonsrc/runtime/internal/app.tssrc/runtime/nitro.tssrc/types/fetch/fetch.tstest/unit/server-fetch-types.test-d.tstsconfig.jsonvitest.config.ts
| typecheck: { | ||
| ignoreSourceErrors: true, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Vitest 4.1.8, what does test.typecheck.ignoreSourceErrorsdo duringvitest --typecheck.only? Does it suppress TypeScript errors originating from imported source files used by *.test-d.ts tests?
💡 Result:
In Vitest 4.1.8, the typecheck.ignoreSourceErrors option is used to prevent the test suite from failing when TypeScript errors are detected in your project's source files [1][2]. By default, Vitest treats source code errors as test failures, even if the errors are not within your *.test-d.ts test files [1][3]. Yes, typecheck.ignoreSourceErrors effectively suppresses TypeScript errors that originate from imported source files [1][2]. When enabled, Vitest will not fail the suite due to these "non-test" errors, and it will essentially hide them from the output [1][2]. This is particularly useful when your type tests (e.g., in *.test-d.ts files) depend on or extend global types that may cause compilation errors in your source code, but you want to focus exclusively on the success of your type assertions [3][4]. When running vitest --typecheck.only, Vitest performs static analysis on your project using a type checker like tsc or vue-tsc [3]. Enabling typecheck.ignoreSourceErrors ensures that the results of this analysis do not cause a non-zero exit code if only source files (as opposed to actual type tests) contain errors [1][5]. Note that this behavior is intended to allow you to run type tests in environments where source code might have unresolved type issues [4][5].
Citations:
- 1: https://vitest.dev/config/typecheck
- 2: https://docs.w3cub.com/vitest/config/typecheck
- 3: https://vitest.dev/guide/testing-types
- 4:
ignoreSourceErrorsis ignored whenrunning typecheck vitest-dev/vitest#4981 - 5: vitest-dev/vitest@6dae3feb
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'FILES:\n'
git ls-files | rg '(^|/)(vitest\.config\.ts|package\.json|.*\.test-d\.ts)$'
printf '\nVITEST CONFIG:\n'
cat -n vitest.config.ts
printf '\nTYPECHECK REFERENCES:\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'typecheck|serverFetch|test-d\.ts' .Repository: nitrojs/nitro
Length of output: 10955
Keep source-file errors visible in the type suite
ignoreSourceErrors: true suppresses diagnostics from imported source files, so a regression in src/types/fetch/fetch.ts can still pass if test/unit/server-fetch-types.test-d.ts parses. Remove this here, or run source typechecks separately from vitest --typecheck.only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vitest.config.ts` around lines 13 - 15, The Vitest typecheck config is hiding
diagnostics from imported source files because `typecheck.ignoreSourceErrors` is
enabled, so source regressions can slip through even when the test d.ts files
pass. Update `vitest.config.ts` to stop suppressing source-file errors in the
`typecheck` block, and if you still need separate source validation, move that
responsibility to a dedicated source typecheck step rather than `vitest
--typecheck.only`.
p-dubovitsky
force-pushed
the
typed-server-fetch
branch
from
135b0f3 to
000d7d1
Compare
p-dubovitsky
force-pushed
the
typed-server-fetch
branch
from
000d7d1 to
c3a088f
Compare
|
Hi @p-dubovitsky really appreciate what you are working on! ❤️ Just a headsup since don't want your efforts to be wasted. It is very likely we will extract typed fetch into a nitro module out of the core before RC. I think we might use parts of your code but not fully. In meantime i will convert PR to draft. (please feel free to drop me a DM in X or Discord (pi0) if wanted to chat. |
2 participants
🔗 Linked issue
Partially addresses #2758
❓ Type of change
📚 Description
This PR adds generated-route type inference for
serverFetch.serverFetchnow uses the generated Nitro route declarations to infer response JSON types for known routes and narrow themethodoption to methods supported by the matched route. Unknown routes still fall back tounknown, preserving compatibility for dynamic or untyped paths.This is intended to be runtime-compatible, but it may expose type-level incompatibilities in code that relied on overly broad
serverFetchinputs. In particular, known routes now narrow accepted methods to the methods declared by the route, andURL/Requestare no longer part of the typedserverFetchrequest input.The PR also adds Vitest type tests covering response inference, method narrowing, and unknown-route fallback against the existing test fixture app.
Documentation and the
server-fetchexample were updated to useserverFetchas the recommended API for internal server-to-server route calls. A separate note documents the exportedfetchhelper as a broader fetch-compatible API that can still use the internal server fetch mechanism for relative Nitro routes, while keepingserverFetchas the typed route-aware API.Typed
fetch/$fetch, route input typing, and serialization-related improvements remain follow-up work for #2758.📝 Checklist