fix: allow to express dependencies between serialization adapters #5314
Conversation
WalkthroughAdds nested serialization adapters and a new SSR route (/ssr/nested) in the e2e app, updates tests, deduplicates adapter registration, and extends the core serializer to support adapter “extends” chains. Adjusts start-server response header assembly and performs a small type cleanup in start-client-core. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router
participant Route(/ssr/nested)
participant Serializer
participant Client
User->>Router: Request /ssr/nested
Router->>Route(/ssr/nested): beforeLoad()
Route(/ssr/nested)->>Route(/ssr/nested): makeNested() -> NestedOuter(NestedInner)
Route(/ssr/nested)->>Serializer: serialize loader/context data
Note right of Serializer: Uses adapters with extends chain
Serializer-->>Router: Serialized payload
Router-->>User: SSR HTML + serialized data
User->>Client: Hydrate
Client->>Serializer: deserialize payload
Client->>Route(/ssr/nested): component() reads loader data
Route(/ssr/nested)-->>User: Render expected vs actual shout/whisper
sequenceDiagram
autonumber
participant Start as Start.createStart
participant Opts as getOptions()
participant Dedupe as dedupeSerializationAdapters
participant Adapter as nestedOuterAdapter
participant Ext as nestedInnerAdapter
Start->>Opts: initialize with options
Opts->>Dedupe: dedupe(options.serializationAdapters)
Dedupe->>Adapter: visit
Adapter->>Ext: visit extends[]
Ext-->>Dedupe: add
Adapter-->>Dedupe: add
Dedupe-->>Opts: deduped list
Opts-->>Start: options with deduped adapters
sequenceDiagram
autonumber
participant Core as router-core serializer
participant Plugin as makeSerovalPlugin()
participant OA as OuterAdapter
participant IA as InnerAdapter
Core->>Plugin: create plugin for OA
Plugin->>OA: test(value)
OA->>OA: toSerializable(value) -> inner instance
Plugin->>Plugin: recurse into extends[]
Plugin->>IA: test(inner)
IA->>IA: toSerializable(inner) -> string
IA-->>Plugin: fromSerializable(string) -> inner
Plugin-->>OA: fromSerializable(inner) -> outer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit f770758
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-server-core/src/createStartHandler.ts (1)
41-51: Don’t drop headers coming from the shared response context.By removing
getResponseHeaders()we no longer flush headers that middleware/routes push viasetResponseHeaders/setResponseHeader(e.g. cookies, CORS). Only mergingmatch.headersmeans those headers disappear from the final SSR response, which breaks auth flows and any route-level header customization. Please restore the merge with the shared response headers (or otherwise propagate them) before release.
🧹 Nitpick comments (4)
packages/router-core/src/ssr/serializer/transformer.ts (2)
32-44: createSerializationAdapter: good API shape; consider avoiding double castThe function shape with
constgeneric forTExtendsAdaptersis solid. Minor: theopts as unknown as SerializationAdapter<...>cast sidelines type errors. If feasible, tighten the interface sooptsalready satisfiesSerializationAdapter<...>(e.g., by intersectingCreateSerializationAdapterOptionswith the required function types), removing the double cast.-export function createSerializationAdapter<...>(opts: CreateSerializationAdapterOptions<...>): SerializationAdapter<...> { - return opts as unknown as SerializationAdapter<...> -} +export function createSerializationAdapter<...>( + opts: CreateSerializationAdapterOptions<...>, +): SerializationAdapter<...> { + return opts +}
139-170: Plugin ‘extends’ mapping works; avoid mutable Array castRuntime wiring is correct. Minor nit: prefer not casting to mutable
Array—useReadonlyArrayto reflect the declared type and avoid unnecessaryas:-extends: serializationAdapter.extends - ? (serializationAdapter.extends as Array<AnySerializationAdapter>).map( - (ext) => makeSsrSerovalPlugin(ext, options), - ) - : undefined, +extends: serializationAdapter.extends + ? (serializationAdapter.extends as ReadonlyArray<AnySerializationAdapter>).map( + (ext) => makeSsrSerovalPlugin(ext, options), + ) + : undefined,packages/start-client-core/src/createStart.ts (1)
120-127: Dedupe invocation: prefer ReadonlyArray and avoid double castThe new dedupe is valuable. Minor type hygiene: accept
ReadonlyArray<AnySerializationAdapter>in the helper to removeas unknown as Array<...>and keep options immutable.-const deduped = new Set<AnySerializationAdapter>() -dedupeSerializationAdapters( - deduped, - options.serializationAdapters as unknown as Array<AnySerializationAdapter>, -) -options.serializationAdapters = Array.from(deduped) as any +const deduped = new Map<string, AnySerializationAdapter>() +dedupeSerializationAdapters(deduped, options.serializationAdapters as ReadonlyArray<AnySerializationAdapter>) +options.serializationAdapters = Array.from(deduped.values()) as anye2e/react-start/serialization-adapters/src/data.tsx (1)
112-118: Consider adding a type guard for consistency.The adapter correctly demonstrates the new
extendsfeature by depending onnestedInnerAdapter. However, for consistency withnestedInnerAdapter(line 107), consider adding an explicit type guard to thetestfunction.Apply this diff:
export const nestedOuterAdapter = createSerializationAdapter({ key: 'nestedOuter', extends: [nestedInnerAdapter], - test: (value) => value instanceof NestedOuter, + test: (value): value is NestedOuter => value instanceof NestedOuter, toSerializable: (outer) => outer.inner, fromSerializable: (value) => new NestedOuter(value), })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
e2e/react-start/serialization-adapters/src/data.tsx(1 hunks)e2e/react-start/serialization-adapters/src/routeTree.gen.ts(6 hunks)e2e/react-start/serialization-adapters/src/routes/ssr/nested.tsx(1 hunks)e2e/react-start/serialization-adapters/src/start.tsx(1 hunks)e2e/react-start/serialization-adapters/tests/app.spec.ts(1 hunks)packages/router-core/src/ssr/serializer/transformer.ts(4 hunks)packages/start-client-core/src/createMiddleware.ts(1 hunks)packages/start-client-core/src/createStart.ts(2 hunks)packages/start-server-core/src/createStartHandler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/serialization-adapters/src/routeTree.gen.tse2e/react-start/serialization-adapters/src/routes/ssr/nested.tsxpackages/start-client-core/src/createStart.tse2e/react-start/serialization-adapters/src/start.tsxe2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/serialization-adapters/src/data.tsxpackages/start-server-core/src/createStartHandler.tspackages/router-core/src/ssr/serializer/transformer.tspackages/start-client-core/src/createMiddleware.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/serialization-adapters/src/routeTree.gen.tse2e/react-start/serialization-adapters/src/routes/ssr/nested.tsxe2e/react-start/serialization-adapters/src/start.tsxe2e/react-start/serialization-adapters/tests/app.spec.tse2e/react-start/serialization-adapters/src/data.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/serialization-adapters/src/routes/ssr/nested.tsx
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-client-core/src/createStart.tspackages/start-server-core/src/createStartHandler.tspackages/start-client-core/src/createMiddleware.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/ssr/serializer/transformer.ts
🧬 Code graph analysis (6)
e2e/react-start/serialization-adapters/src/routeTree.gen.ts (2)
e2e/react-start/basic/src/routeTree.gen.ts (3)
FileRoutesByTo(300-332)FileRoutesById(333-375)RootRouteChildren(493-511)e2e/react-router/js-only-file-based/src/routeTree.gen.js (1)
IndexRoute(30-34)
e2e/react-start/serialization-adapters/src/routes/ssr/nested.tsx (1)
e2e/react-start/serialization-adapters/src/data.tsx (1)
makeNested(120-122)
packages/start-client-core/src/createStart.ts (2)
packages/router-core/src/ssr/serializer/transformer.ts (1)
AnySerializationAdapter(137-137)packages/router-core/src/index.ts (1)
AnySerializationAdapter(416-416)
e2e/react-start/serialization-adapters/src/start.tsx (2)
packages/start-client-core/src/createStart.ts (1)
createStart(83-137)e2e/react-start/serialization-adapters/src/data.tsx (1)
nestedOuterAdapter(112-118)
e2e/react-start/serialization-adapters/src/data.tsx (1)
packages/router-core/src/ssr/serializer/transformer.ts (1)
createSerializationAdapter(30-44)
packages/router-core/src/ssr/serializer/transformer.ts (1)
packages/router-core/src/index.ts (7)
AnySerializationAdapter(416-416)createSerializationAdapter(429-429)SerializationAdapter(417-417)ValidateSerializable(421-421)Serializable(425-425)makeSsrSerovalPlugin(431-431)makeSerovalPlugin(430-430)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (25)
packages/start-client-core/src/createMiddleware.ts (1)
292-298: Safer context merge without wideningSwitching the inner merge to
Assign<GlobalServerRequestContext<TRegister>, …>keeps the registered server request context narrow instead of immediately widening toAnyContext, so downstream middleware gets the proper inference without losing the existing fallback semantics. Looks great.packages/router-core/src/ssr/serializer/transformer.ts (6)
26-29: Type-level union of extended adapters’ inputs looks correct
UnionizeSerializationAdaptersInputcleanly unions['~types']['input']from the extends chain; withneverit collapses as intended. LGTM.
46-61: toSerializable typing: nice expansion to accept extended adapters’ inputsWidening
ValidateSerializabletoSerializable | Unionize...is the right move to enable chaining. No issues spotted.
114-125: SerializationAdapter ‘extends’ typing is precise; good use ofneverto forbid propertyThe
extends?: TExtendsAdapterswithTExtendsAdapters = neverelegantly prevents accidental assignment. LGTM.
127-135: ‘~types’.input includes parent and extended inputsIncluding
UnionizeSerializationAdaptersInput<TExtendsAdapters>ininputenables downstream composition. Looks good.
137-137: AnySerializationAdapter now parameterizes all three genericsThis aligns consumers (e.g., start-client-core) with the new shape. LGTM.
172-201: Client plugin deserialization path looks correct
fromSerializable(ctx.deserialize(node))matches the chaining model; parse variants cover sync/async/stream. LGTM.If we ever allow deep (>1) extends chains, consider adding an e2e that nests 3+ adapters to validate multi-hop parse/deserialize.
e2e/react-start/serialization-adapters/src/start.tsx (2)
2-2: Import of nestedOuterAdapter only is appropriateMatches the intent to register
nestedInnerAdapterviaextendson the outer adapter. LGTM.
8-14: Unable to automatically verify that all createSerializationAdapter keys are unique. Please manually review every adapter definition (including those brought in viaextends) to ensure no duplicate keys exist.packages/start-client-core/src/createStart.ts (1)
83-137: Public type imports align with router-core changesAnySerializationAdapter usage matches the updated alias in router-core. LGTM.
Please confirm the repository’s TypeScript version is ≥ 5.3 (required for const type parameters).
e2e/react-start/serialization-adapters/tests/app.spec.ts (1)
53-72: LGTM!The test case correctly validates nested serialization adapter functionality by comparing expected (locally generated) vs actual (server-serialized) state for both
shoutandwhispermethods. The structure follows the established testing pattern.e2e/react-start/serialization-adapters/src/routes/ssr/nested.tsx (3)
5-10: LGTM!The
beforeLoadhook correctly seeds the context with nested data, and the loader propagates the entire context for serialization testing. This pattern is appropriate for validating SSR serialization round-trips.
11-19: LGTM!The component logic correctly compares locally-generated expected values against loader-provided actual values, validating that serialization/deserialization preserves both the object structure and method behavior.
20-54: LGTM!The render output correctly exposes expected and actual state with test IDs that match the test assertions in
app.spec.ts. The structure facilitates both visual debugging and automated testing.e2e/react-start/serialization-adapters/src/data.tsx (4)
98-103: LGTM!The
NestedInnerclass provides a simple string wrapper with anshout()method for testing. The implementation is clean and correct.
91-96: LGTM!The
NestedOuterclass correctly wrapsNestedInnerand provides awhisper()method. This nested structure is appropriate for testing serialization adapter dependency chains.
105-110: LGTM!The
nestedInnerAdaptercorrectly serializesNestedInnerinstances to their string values and reconstructs them during deserialization. The type guard is properly specified.
120-122: LGTM!The
makeNested()factory correctly instantiates the nested structure for testing. The fixed seed value ('Hello World') ensures consistent test results.e2e/react-start/serialization-adapters/src/routeTree.gen.ts (7)
14-14: LGTM: Import statement follows existing pattern.The import for the new nested route is consistent with other SSR route imports in this auto-generated file.
28-32: LGTM: Route definition is consistent with existing routes.The SsrNestedRoute definition correctly follows the established pattern with proper id, path, and parent route configuration.
49-49: LGTM: Type interface additions are correct.The new route is properly added to all three route mapping interfaces (FileRoutesByFullPath, FileRoutesByTo, and FileRoutesById) with consistent typing.
Also applies to: 56-56, 64-64
73-73: LGTM: Union type additions are correct.The new route path '/ssr/nested' is properly added to the fullPaths, to, and id union types in FileRouteTypes. The reformatting of the
tounion type (lines 76-81) maintains consistency while adding the new route.Also applies to: 76-81, 87-87
95-95: LGTM: RootRouteChildren interface correctly updated.The SsrNestedRoute is properly added to the RootRouteChildren interface with correct typing.
115-121: LGTM: Module augmentation is complete and correct.The module augmentation for '@tanstack/react-router' properly declares the '/ssr/nested' route with all required metadata (id, path, fullPath, preLoaderRoute, parentRoute).
143-143: LGTM: Route tree construction properly includes the new route.The rootRouteChildren object correctly includes the SsrNestedRoute, completing the integration of the new '/ssr/nested' route into the route tree.
| function dedupeSerializationAdapters( | ||
| deduped: Set<AnySerializationAdapter>, | ||
| plugins: Array<AnySerializationAdapter>, | ||
| ): void { | ||
| for (let i = 0, len = plugins.length; i < len; i++) { | ||
| const current = plugins[i]! | ||
| if (!deduped.has(current)) { | ||
| deduped.add(current) | ||
| if (current.extends) { | ||
| dedupeSerializationAdapters(deduped, current.extends) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Dedupe by identity only; also dedupe by key to prevent registry collisions
Current Set dedupes by object identity. Two distinct adapter instances with the same key can still both be retained, leading to ambiguous registration (last one wins) and hard-to-debug behavior.
Consider deduping by key (in addition to identity) so only the first occurrence is kept:
-function dedupeSerializationAdapters(
- deduped: Set<AnySerializationAdapter>,
- plugins: Array<AnySerializationAdapter>,
-): void {
- for (let i = 0, len = plugins.length; i < len; i++) {
- const current = plugins[i]!
- if (!deduped.has(current)) {
- deduped.add(current)
- if (current.extends) {
- dedupeSerializationAdapters(deduped, current.extends)
- }
- }
- }
-}
+function dedupeSerializationAdapters(
+ seenByKey: Map<string, AnySerializationAdapter>,
+ plugins: ReadonlyArray<AnySerializationAdapter>,
+): void {
+ for (let i = 0, len = plugins.length; i < len; i++) {
+ const current = plugins[i]!
+ if (!seenByKey.has(current.key)) {
+ seenByKey.set(current.key, current)
+ if (current.extends) {
+ dedupeSerializationAdapters(seenByKey, current.extends as ReadonlyArray<AnySerializationAdapter>)
+ }
+ }
+ }
+}And update the call site to use Array.from(map.values()) to preserve first-seen order.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/start-client-core/src/createStart.ts around lines 68 to 81, the
current dedupeSerializationAdapters function only dedupes by object identity
which allows multiple distinct adapter objects with the same key to remain and
cause registry collisions; change the implementation to dedupe by adapter.key in
addition to identity (for example, track seen keys in a Set or use a Map keyed
by adapter.key to keep the first-seen instance and ignore subsequent ones),
ensure recursion over current.extends uses the same dedupe logic, and update the
call site that collects final adapters to use Array.from(map.values()) (or
otherwise preserve first-seen order) so only the initial adapter per key is kept
and ordering is stable.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests