Uh oh!
There was an error while loading. Please reload this page.
fix(start): align request handler types with registered server context - #7428
fix(start): align request handler types with registered server context#7428SeanCassiere wants to merge 10 commits into
Conversation
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR aligns request handler types across the start framework packages by updating where the ChangesRequest Handler Type Alignment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
View your CI Pipeline Execution ↗ for commit bfdf4bb
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview5 package(s) bumped directly, 9 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will not alter performance
Comparing  Footnotes
|
89310e0 to
d6b0635CompareThere was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/start-server-core/src/createStartHandler.ts`:
- Around line 325-327: startRequestResolver is declared as
RequestHandler<Register> with requestOpts?: RequestOptions<Register>, but the
code later casts it to RequestHandler<TRegister>, which hides
RequestOptions<TRegister> constraints such as required server.requestContext;
instead make startRequestResolver generic over TRegister (or accept
RequestOptions<TRegister>) within createStartHandler so its signature matches
RequestHandler<TRegister> without casting—update the function/type parameters
(startRequestResolver, RequestOptions, RequestHandler, createStartHandler,
TRegister, Register) so the resolver uses the generic TRegister request option
types (including requestContext when present) and remove the unsafe cast.
In `@packages/start-server-core/src/request-response.ts`:
- Around line 125-128: The wrapper currently declares requestOpts?: any which
bypasses the RequestHandler<TRegister> contract and then re-establishes types
via a cast; change the wrapper signature to accept the strongly-typed options
instead of any so the wrapper itself matches RequestHandler<TRegister> (i.e.,
replace requestOpts?: any with the correct generic opts type for TRegister and
adjust the wrapper return type to Promise<Response> | Response as needed),
remove the subsequent as RequestHandler<TRegister> cast, and ensure the internal
call sites use the typed opts variable (references: the wrapper function taking
(request: Request, requestOpts), the RequestHandler<TRegister> type and the
TRegister generic).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7286b653-e8ba-4a37-ae1f-5478603a1e1f
📒 Files selected for processing (8)
.changeset/thick-sloths-write.mdpackages/react-start/src/default-entry/server.tspackages/solid-start/src/default-entry/server.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/index.tsxpackages/start-server-core/src/request-handler.tspackages/start-server-core/src/request-response.tspackages/vue-start/src/default-entry/server.ts
✅ Files skipped from review due to trivial changes (2)
- packages/start-server-core/src/index.tsx
- .changeset/thick-sloths-write.md
Uh oh!
There was an error while loading. Please reload this page.
| return (( | ||
| request: Request, | ||
| requestOpts?: any, | ||
| ): Promise<Response> | Response => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Verify unsafe typing in wrapper signatures
rg -n -C2 'requestOpts\?: any|as RequestHandler<|RequestHandlerParameters<' packages/start-server-core/src/request-response.tsRepository: TanStack/router
Length of output: 342
requestOpts?: any bypasses the handler type guarantees (and is masked by a cast).
In packages/start-server-core/src/request-response.ts, the wrapper takes requestOpts?: any (line 127) and then relies on as RequestHandler<TRegister> (line 146) to restore typing. This defeats strict type safety for the TRegister-specific opts contract. Replace any with the correct typed parameters for TRegister and remove/avoid the cast by aligning the wrapper’s parameter/return types with RequestHandler<TRegister] directly.
🤖 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 `@packages/start-server-core/src/request-response.ts` around lines 125 - 128,
The wrapper currently declares requestOpts?: any which bypasses the
RequestHandler<TRegister> contract and then re-establishes types via a cast;
change the wrapper signature to accept the strongly-typed options instead of any
so the wrapper itself matches RequestHandler<TRegister> (i.e., replace
requestOpts?: any with the correct generic opts type for TRegister and adjust
the wrapper return type to Promise<Response> | Response as needed), remove the
subsequent as RequestHandler<TRegister> cast, and ensure the internal call sites
use the typed opts variable (references: the wrapper function taking (request:
Request, requestOpts), the RequestHandler<TRegister> type and the TRegister
generic).
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…t-start Add a Start-owned Register interface that extends router-core's Register, then export and consume it from `start-client-core`. This lets declarations against `@tanstack/react-start`, including `server.requestContext`, flow into Start-specific type consumers instead of requiring users to augment `@tanstack/router-core` or `@tanstack/react-router` directly. Router-core Register augmentations remain compatible because Start's Register extends the core Register.
Import Register from `@tanstack/start-client-core` in `createStartHandler` instead of `@tanstack/router-core`. This makes the server handler default to Start's augmentation surface, allowing `@tanstack/react-start` Register declarations such as `server.requestContext` to flow into RequestHandler typing.
Use the shared router-core Register for Start request handler types while preserving the documented custom server entry shape. Request options now derive required context from Register.server.requestContext, but framework ServerEntry wrappers still accept the universal fetch handler form with optional request options.
73a9c6d to
bfdf4bbCompare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
bd4dd3f to
bfdf4bbCompare
In reference to #7399
Summary by CodeRabbit
Bug Fixes
Documentation