fix(router-core): avoid infinite serializer recursion for arrays #5199
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughExtend serializer type utilities to handle Changes
Sequence Diagram(s)(omitted — changes are type-level/type-system dispatches without runtime control flow to diagram) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes infinite recursion issues in the TypeScript serializer by adding specialized handling for arrays in the type validation system.
- Adds explicit array handling to prevent infinite type recursion when validating serializable types
- Introduces helper types to distinguish between tuples and regular arrays for proper serialization
- Includes regression test to ensure recursive payload types don't cause infinite type expansion
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/router-core/src/ssr/serializer/transformer.ts | Adds array-specific validation types and helper utilities to handle recursion |
| packages/router-core/tests/serializer-recursion.test-d.ts | Adds type-level test for recursive array serialization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
brhx
force-pushed
the
fix-infinite-type
branch
from
150501f to
3777733
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
10-13: Consider adding tuple and readonly variants to harden coverageAdd similar assertions for tuples and readonly arrays to ensure tuple structure and readonly-ness are preserved.
Example additions:
- type ResultTuple = readonly [ResultTuple, { [k: string]: ResultTuple }]
- expect ValidateSerializableResult<ResultTuple, Serializable> equals ResultTuple
packages/router-core/src/ssr/serializer/transformer.ts (3)
35-49: Make validation distributive over unions to hit the array branch inside unionsCurrently, unions like Array | Y won’t enter the array path at the top level, falling back to the object mapping. Making the conditional distributive ensures each union member takes the optimal branch (array vs. object), further reducing expansion risk.
Apply this diff:
-export type ValidateSerializable<T, TSerializable> = T extends TSerializable - ? T - : T extends ReadonlyArray<any> - ? ValidateSerializableArray<T, TSerializable> - : T extends (...args: Array<any>) => any - ? 'Function is not serializable' - : T extends Promise<any> - ? ValidateSerializablePromise<T, TSerializable> - : T extends ReadableStream<any> - ? ValidateReadableStream<T, TSerializable> - : T extends Set<any> - ? ValidateSerializableSet<T, TSerializable> - : T extends Map<any, any> - ? ValidateSerializableMap<T, TSerializable> - : { - [K in keyof T]: ValidateSerializable<T[K], TSerializable> - } +export type ValidateSerializable<T, TSerializable> = + T extends unknown + ? T extends TSerializable + ? T + : T extends ReadonlyArray<any> + ? ValidateSerializableArray<T, TSerializable> + : T extends (...args: Array<any>) => any + ? 'Function is not serializable' + : T extends Promise<any> + ? ValidateSerializablePromise<T, TSerializable> + : T extends ReadableStream<any> + ? ValidateReadableStream<T, TSerializable> + : T extends Set<any> + ? ValidateSerializableSet<T, TSerializable> + : T extends Map<any, any> + ? ValidateSerializableMap<T, TSerializable> + : { [K in keyof T]: ValidateSerializable<T[K], TSerializable> } + : never
81-85: Minor: simplify tuple detection (optional)A common alternative is using number extends T['length'] to detect non‑tuples. Your current approach is fine; this is stylistic.
Possible form:
- type IsTuple<T extends ReadonlyArray> = number extends T['length'] ? false : true
193-200: Also distribute over unions for result validationSame rationale as for ValidateSerializable: ensure Array | U hits the array path for the array member.
Apply this diff:
-export type ValidateSerializableResult<T, TSerializable> = - T extends TSerializable - ? T - : T extends ReadonlyArray<any> - ? ValidateSerializableResultArray<T, TSerializable> - : unknown extends SerializerExtensions['ReadableStream'] - ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } - : T extends SerializerExtensions['ReadableStream'] - ? ReadableStream - : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } +export type ValidateSerializableResult<T, TSerializable> = + T extends unknown + ? T extends TSerializable + ? T + : T extends ReadonlyArray<any> + ? ValidateSerializableResultArray<T, TSerializable> + : unknown extends SerializerExtensions['ReadableStream'] + ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } + : T extends SerializerExtensions['ReadableStream'] + ? ReadableStream + : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } + : never
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)packages/router-core/tests/serializer-recursion.test-d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
🧬 Code graph analysis (1)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
packages/router-core/src/ssr/serializer/transformer.ts (2)
ValidateSerializableResult(190-199)Serializable(8-15)
🔇 Additional comments (5)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
8-15: Good addition: validates recursive array/object without blow‑upThis d.ts test captures the regression well and guards against infinite expansion in recursive array/object unions.
packages/router-core/src/ssr/serializer/transformer.ts (4)
35-49: Array/tuple short‑circuit prevents mapped‑type explosion on arraysSpecial‑casing ReadonlyArray before the object mapping is the right fix to avoid recursing into array methods/properties.
74-86: Tuple‑aware array validation looks solidCorrectly preserves tuple shapes and handles mutable vs. readonly arrays without widening.
193-200: Array short‑circuit in result type avoids recursive expansion via array methodsRouting arrays to ValidateSerializableResultArray here prevents mapped‑type recursion into array members/methods. Good ordering with the ReadableStream branch.
201-209: Result array handling preserves tuple shapes and readonly correctlyThe element‑wise recursion mirrors the non‑result path appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
8-15: Add tuple and readonly-array cases to prevent regressionsCover tuples and readonly arrays to ensure element-wise preservation and mutability is respected.
You can append tests like:
it('should preserve tuples and readonly arrays', () => { type Result = Array<Result> | { [key: string]: Result } type Tup = readonly [Result, Result] expectTypeOf<ValidateSerializableResult<Tup, Serializable>>() .branded.toEqualTypeOf<Tup>() type RArr = readonly Result[] expectTypeOf<ValidateSerializableResult<RArr, Serializable>>() .branded.toEqualTypeOf<RArr>() })packages/router-core/src/ssr/serializer/transformer.ts (4)
35-49: Prefer unknown over any in array/stream/set/map branchesUsing unknown avoids accidental any propagation and tightens strict-mode guarantees without changing behavior.
Apply:
- : T extends ReadonlyArray<any> + : T extends ReadonlyArray<unknown> ? ValidateSerializableArray<T, TSerializable> - : T extends (...args: Array<any>) => any + : T extends (...args: any[]) => unknown ? 'Function is not serializable' - : T extends Promise<any> + : T extends Promise<unknown> ? ValidateSerializablePromise<T, TSerializable> - : T extends ReadableStream<any> + : T extends ReadableStream<unknown> ? ValidateReadableStream<T, TSerializable> - : T extends Set<any> + : T extends Set<unknown> ? ValidateSerializableSet<T, TSerializable> - : T extends Map<any, any> + : T extends Map<unknown, unknown> ? ValidateSerializableMap<T, TSerializable> : { [K in keyof T]: ValidateSerializable<T[K], TSerializable> }
74-80: Array validation utility looks correct; align generics with unknownLogic is solid. Minor tightening: switch ReadonlyArray to ReadonlyArray.
-type ValidateSerializableArray<T extends ReadonlyArray<any>, TSerializable> = +type ValidateSerializableArray<T extends ReadonlyArray<unknown>, TSerializable> = IsTuple<T> extends true ? { [K in keyof T]: ValidateSerializable<T[K], TSerializable> } : T extends Array<infer U> ? Array<ValidateSerializable<U, TSerializable>> : ReadonlyArray<ValidateSerializable<T[number], TSerializable>>
81-86: IsTuple helper is fine; alternative idiom optionalCurrent form is clear. Optionally, the length-test variant is concise:
[number extends T['length'] ? false : true]
201-209: Mirror unknown usage in ResultArray for consistencyAlign with earlier unknown adjustments to keep strictness consistent.
-type ValidateSerializableResultArray< - T extends ReadonlyArray<any>, +type ValidateSerializableResultArray< + T extends ReadonlyArray<unknown>, TSerializable, > = IsTuple<T> extends true ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } : T extends Array<infer U> ? Array<ValidateSerializableResult<U, TSerializable>> : ReadonlyArray<ValidateSerializableResult<T[number], TSerializable>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)packages/router-core/tests/serializer-recursion.test-d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
🧬 Code graph analysis (1)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
packages/router-core/src/ssr/serializer/transformer.ts (2)
ValidateSerializableResult(190-199)Serializable(8-15)
🔇 Additional comments (4)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
10-13: Type-level recursion test reads well and asserts the intended fixThis correctly exercises the recursive array/object shape and ensures no infinite expansion.
packages/router-core/src/ssr/serializer/transformer.ts (3)
190-200: Result validator: array branch unblocks recursion — LGTMThe ReadonlyArray branch prevents runaway expansion and composes with object mapping. Good fix.
74-80: Optional: dedupe ValidateSerializableArray and ValidateSerializableResultArrayThere’s some repetition between the two array helpers. If you ever want to DRY this up, a shared internal helper can parameterize the inner mapper.
Also applies to: 201-209
74-80: Export surface: confirm intent to keep helpers internalThese helpers aren’t exported. If you intend external consumption (docs suggest they might be useful), consider exporting; otherwise, keeping them internal is fine.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/ssr/serializer/transformer.ts (1)
204-212: Verify the duplicate validation logic concern is addressed.The
ValidateSerializableResultArraytype duplicates the same logic pattern asValidateSerializableArray(Lines 75-81). While the previous reviewer suggested extracting shared logic and the author declined, consider if this duplication could lead to maintenance issues if the validation logic needs to change.The duplication exists between:
ValidateSerializableArray(Lines 75-81)ValidateSerializableResultArray(Lines 204-212)Both follow the identical pattern:
IsTuple<T>check for element-wise mappingArray<infer U>check for homogeneous arraysReadonlyArrayfallbackIf this validation logic needs updates in the future, both types must be kept in sync manually.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)packages/router-core/tests/serializer-recursion.test-d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/tests/serializer-recursion.test-d.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (5)
packages/router-core/src/ssr/serializer/transformer.ts (5)
33-50: LGTM! Improved array handling in serialization validation.The addition of
ReadonlyArray<any>handling throughValidateSerializableArraycorrectly addresses the infinite recursion issue mentioned in the PR objectives. The type now properly routes readonly arrays, arrays, and tuples through dedicated validation logic instead of falling through to the generic object mapping.
75-81: Well-structured array validation with proper tuple/array distinction.The implementation correctly handles three cases:
- Tuples: Element-wise mapping preserving structure
- Regular arrays: Homogeneous element validation
- Readonly arrays: Readonly element validation
This addresses the core issue while maintaining type safety for different array types.
82-87: Effective tuple detection logic.The
IsTupletype correctly identifies tuples by checking for empty arrays or arrays with at least one element followed by rest parameters. This distinction is crucial for proper array handling in the validation logic.
191-203: Consistent array handling in result validation.The
ValidateSerializableResulttype now mirrors the input validation logic by routingReadonlyArray<any>throughValidateSerializableResultArray. This ensures consistency between input and result validation paths.
37-38: ValidateSerializableResult already covered for ReadonlyArray — add/confirm equivalent tests for ValidateSerializable (input)
- Existing coverage: packages/router-core/tests/serializer-recursion.test-d.ts (recursive Array, tuple, and ReadonlyArray cases for ValidateSerializableResult).
- Missing: no tests found that exercise ValidateSerializable<..., Serializable> ReadonlyArray paths — add type-level tests mirroring the result cases or confirm they exist elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/router-core/src/ssr/serializer/transformer.ts (5)
33-49: Array branch fixes the recursion. Consider narrowinganytounknownfor safer constraints.Using
unknownavoids accidentalanyleakage and improves type inference without changing behavior.Apply this diff:
- : T extends ReadonlyArray<any> + : T extends ReadonlyArray<unknown> ? ValidateSerializableArray<T, TSerializable>
82-92: Preferunknownoveranyin array core to preventanypropagation.Safer generic bounds; no behavioral change.
-type ValidateSerializableArrayCore< - T extends ReadonlyArray<any>, +type ValidateSerializableArrayCore< + T extends ReadonlyArray<unknown>, TSerializable, TKind extends 'input' | 'result', > = IsTuple<T> extends true ? { [K in keyof T]: ApplyArrayValidation<T[K], TSerializable, TKind> } : T extends Array<infer U> ? Array<ApplyArrayValidation<U, TSerializable, TKind>> : ReadonlyArray<ApplyArrayValidation<T[number], TSerializable, TKind>>
98-103: TightenIsTupletounknownand avoidanyin the tuple probe.Improves soundness and avoids
anyinference.-type IsTuple<T extends ReadonlyArray<any>> = T extends readonly [] +type IsTuple<T extends ReadonlyArray<unknown>> = T extends readonly [] ? true - : T extends readonly [any, ...infer _Rest] - ? true - : false + : T extends readonly [unknown, ...unknown[]] + ? true + : false
207-217: Result-path array specialization looks correct; mirror theunknownrefinement here too.Consistent constraints across input/result branches.
- : T extends ReadonlyArray<any> + : T extends ReadonlyArray<unknown> ? ValidateSerializableResultArray<T, TSerializable>
219-223: Consistent reuse of the array core for results is solid. Considerunknownbound here as well.Keeps constraints aligned with other helpers.
-type ValidateSerializableResultArray< - T extends ReadonlyArray<any>, +type ValidateSerializableResultArray< + T extends ReadonlyArray<unknown>, TSerializable, > = ValidateSerializableArrayCore<T, TSerializable, 'result'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (2)
packages/router-core/src/ssr/serializer/transformer.ts (2)
74-81: Good consolidation viaApplyArrayValidation.This removes duplication and cleanly switches between input/result validation.
93-97: Alias reads well and keeps public surface minimal.Clear and idiomatic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router-core/src/ssr/serializer/transformer.ts (3)
82-92: Make array validation distributive over unions and more robust for tuple detectionThe current IsTuple check does not distribute over unions, so unions like (readonly [A] | B[]) lose tuple precision. Rewrite the core using a distributive conditional with length inference.
Apply this diff:
-type ValidateSerializableArrayCore< - T extends ReadonlyArray<unknown>, - TSerializable, - TKind extends 'input' | 'result', -> = - IsTuple<T> extends true - ? { [K in keyof T]: ApplyArrayValidation<T[K], TSerializable, TKind> } - : T extends Array<infer U> - ? Array<ApplyArrayValidation<U, TSerializable, TKind>> - : ReadonlyArray<ApplyArrayValidation<T[number], TSerializable, TKind>> +type ValidateSerializableArrayCore< + T extends ReadonlyArray<unknown>, + TSerializable, + TKind extends 'input' | 'result', +> = + T extends any + ? number extends T['length'] + ? T extends Array<infer U> + ? Array<ApplyArrayValidation<U, TSerializable, TKind>> + : ReadonlyArray<ApplyArrayValidation<T[number], TSerializable, TKind>> + : { [K in keyof T]: ApplyArrayValidation<T[K], TSerializable, TKind> } + : never
98-103: Remove unused IsTuple or switch to length-based detectionWith the proposed core rewrite, IsTuple becomes unused. Either remove it or update to the canonical length-based form if you still want it around.
Option A (remove):
-type IsTuple<T extends ReadonlyArray<unknown>> = T extends readonly [] - ? true - : T extends readonly [unknown, ...Array<unknown>] - ? true - : falseOption B (canonical form):
-type IsTuple<T extends ReadonlyArray<unknown>> = T extends readonly [] - ? true - : T extends readonly [unknown, ...Array<unknown>] - ? true - : false +type IsTuple<T extends ReadonlyArray<unknown>> = + number extends T['length'] ? false : true
207-217: Apply the same array-first check in result pathSame precedence concern as the input path: arrays could be short-circuited by T extends TSerializable. Route arrays first.
Apply this diff:
-export type ValidateSerializableResult<T, TSerializable> = T extends unknown - ? T extends TSerializable - ? T - : T extends ReadonlyArray<unknown> - ? ValidateSerializableResultArray<T, TSerializable> +export type ValidateSerializableResult<T, TSerializable> = T extends unknown + ? T extends ReadonlyArray<unknown> + ? ValidateSerializableResultArray<T, TSerializable> + : T extends TSerializable + ? T : unknown extends SerializerExtensions['ReadableStream'] ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } : T extends SerializerExtensions['ReadableStream'] ? ReadableStream : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } : neverIf you decide against reordering, please confirm that no TSerializable passed here admits arrays; otherwise element validation may be skipped for results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (3)
packages/router-core/src/ssr/serializer/transformer.ts (3)
74-81: LGTM: clean indirection for input vs result array validationThe ApplyArrayValidation helper keeps core logic DRY between input/result paths.
219-223: LGTM: result arrays reuse the shared coreResult path correctly reuses the core to preserve tuple shape/readonly.
33-49: Ensure arrays aren’t short-circuited by TSerializableMove the array branch before the
T extends TSerializablecheck so arrays get element-wise validation whenTSerializablecan include array types.File: packages/router-core/src/ssr/serializer/transformer.ts (lines 33-49)
-export type ValidateSerializable<T, TSerializable> = T extends unknown - ? T extends TSerializable - ? T - : T extends ReadonlyArray<unknown> - ? ValidateSerializableArray<T, TSerializable> +export type ValidateSerializable<T, TSerializable> = T extends unknown + ? T extends ReadonlyArray<unknown> + ? ValidateSerializableArray<T, TSerializable> + : T extends TSerializable + ? T : T extends (...args: Array<any>) => any ? 'Function is not serializable' : T extends Promise<any> ? ValidateSerializablePromise<T, TSerializable> : T extends ReadableStream<any> ? ValidateReadableStream<T, TSerializable> : T extends Set<any> ? ValidateSerializableSet<T, TSerializable> : T extends Map<any, any> ? ValidateSerializableMap<T, TSerializable> : { [K in keyof T]: ValidateSerializable<T[K], TSerializable> } : neverAutomated search for
serializationAdaptersreturned no matches because ripgrep reported "No files were searched" — confirm whether any registered serialization adapters acceptArray<...>,ReadonlyArray<...>or tuple types; if so, apply the diff above.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/ssr/serializer/transformer.ts (2)
74-93: Solid dedup via shared array core; consider variadic tuple nuanceValidateSerializableArrayCore + ApplyArrayValidation nicely centralize array logic. One nuance: number extends T['length'] treats variadic tuples (with rest) as non‑tuples, so their precise head/rest structure is widened to (Readonly)Array<…>. If you want to retain variadic tuple fidelity, consider a specialized mapper that peels head elements and preserves rest types; otherwise, this trade‑off is fine.
Can you confirm whether widening variadic tuples is acceptable for your use cases (e.g., readonly [...A[], ...B[]])?
202-212: Add function guard for result path (parity with input path)Currently, a function type on the result path devolves to an empty object type via the indexer branch. For consistency with ValidateSerializable (which returns a diagnostic string for functions) and clearer feedback, add an explicit function check.
Proposed minimal diff:
export type ValidateSerializableResult<T, TSerializable> = T extends unknown ? T extends ReadonlyArray<unknown> ? ValidateSerializableResultArray<T, TSerializable> : T extends TSerializable ? T + : T extends (...args: Array<any>) => any + ? 'Function is not serializable' : unknown extends SerializerExtensions['ReadableStream'] ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } : T extends SerializerExtensions['ReadableStream'] ? ReadableStream : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } : neverIf the current behavior is intentional (functions never appear in result types), feel free to ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (3)
packages/router-core/src/ssr/serializer/transformer.ts (3)
33-49: Array special‑casing removes recursion risk and preserves shapeGood change: routing ReadonlyArray through ValidateSerializableArray prevents infinite recursion and preserves tuple vs non‑tuple and readonly characteristics downstream.
94-98: Thin wrapper reads wellBinding the core to 'input' via ValidateSerializableArray keeps call sites clear.
214-218: Result array path correctly reuses the coreGood reuse of the shared core for the 'result' branch; shape and readonly preservation carry over.
brhx
force-pushed
the
fix-infinite-type
branch
3 times, most recently
from
d8b5e1a to
bf84032
Compare
|
View your CI Pipeline Execution ↗ for commit 7b18dc8
☁️ Nx Cloud last updated this comment at |
brhx
force-pushed
the
fix-infinite-type
branch
from
ecab49d to
669f883
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/ssr/serializer/transformer.ts (1)
78-82: Consider using a type helper for improved readability.The conditional logic for checking array vs tuple (lines 78-82 and 214-218) could be extracted into a reusable type helper for better maintainability.
+type IsTuple<T extends ReadonlyArray<unknown>> = number extends T['length'] ? false : true + type ValidateSerializableArray< T extends ReadonlyArray<unknown>, TSerializable, -> = number extends T['length'] - ? T extends Array<infer U> +> = IsTuple<T> extends true + ? ValidateSerializableTuple<T, TSerializable> + : T extends Array<infer U> ? Array<ValidateSerializable<U, TSerializable>> : ReadonlyArray<ValidateSerializable<T[number], TSerializable>> - : ValidateSerializableTuple<T, TSerializable>Apply the same pattern to
ValidateSerializableResultArrayat lines 214-218.packages/router-core/tests/serializer-recursion.test-d.ts (1)
55-80: Consider adding edge case tests for mixed recursive structures.While the current tests are comprehensive, consider adding a test case for deeply nested mixed structures that combine arrays, tuples, and objects with recursive references.
Add this test case after line 79:
it('should preserve mixed recursive structures without infinite expansion', () => { type MixedRecursive = { array: Array<MixedRecursive> tuple: readonly [MixedRecursive, number] nested: { value: MixedRecursive | null } } expectTypeOf< ValidateSerializableResult<MixedRecursive, Serializable> >().branded.toEqualTypeOf<MixedRecursive>() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)packages/router-core/tests/serializer-recursion.test-d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/serializer-recursion.test-d.tspackages/router-core/src/ssr/serializer/transformer.ts
🧬 Code graph analysis (1)
packages/router-core/tests/serializer-recursion.test-d.ts (1)
packages/router-core/src/ssr/serializer/transformer.ts (3)
ValidateSerializable(33-50)Serializable(8-15)ValidateSerializableResult(200-209)
🔇 Additional comments (7)
packages/router-core/src/ssr/serializer/transformer.ts (4)
33-50: LGTM! Well-structured type validation for arrays.The refactor correctly handles
ReadonlyArraytypes first in the conditional chain, preventing infinite recursion for self-referential array types. This fix addresses the reported issue with recursive array serialization.
75-96: Solid implementation of array and tuple type handling.The type utilities correctly differentiate between regular arrays and tuples using the
number extends T['length']check, and handle both mutable and readonly variants appropriately. The recursive tuple validation preserves the exact shape and ordering of tuple elements.
201-210: Result validation correctly mirrors input validation structure.The
ValidateSerializableResulttype properly handles arrays with the same early-exit pattern asValidateSerializable, ensuring consistency in type validation across both input and result paths.
211-231: Result array/tuple types maintain structural consistency.The implementation correctly preserves the distinction between arrays, readonly arrays, and tuples in the result validation path, maintaining type fidelity throughout the serialization process.
packages/router-core/tests/serializer-recursion.test-d.ts (3)
1-8: LGTM! Test file properly set up for type-level testing.The imports and test structure are appropriate for validating the type-level serialization behavior.
9-54: Comprehensive test coverage for serialization input types.The tests thoroughly validate that
ValidateSerializablepreserves various array structures (nested, tuple, readonly) and wrapped recursive types (Promise, ReadableStream). This ensures the type system correctly handles complex nested structures without modification.
55-80: Critical recursive type tests effectively prevent regression.These tests are particularly important as they verify the fix for the infinite recursion issue (#4533). They ensure that self-referential types like
Array<Result>and recursive tuples are handled without causing TypeScript to enter infinite type expansion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/ssr/serializer/transformer.ts (2)
75-83: Edge case: variadic tuples ([H, ...T[]]) get flattened to plain arraysWhen
number extends T['length'], a tuple with a rest (e.g.,[H, ...T[]]) is treated as an array and loses the fixed head/rest shape. If that trade‑off is intentional for perf/termination, add a brief comment to document it.Apply this diff to document the trade‑off:
type ValidateSerializableArray< T extends ReadonlyArray<unknown>, TSerializable, > = number extends T['length'] + /** + * Note: Tuple types with rest elements (e.g. [Head, ...Tail[]]) + * are treated as arrays to avoid pathological recursion and cost. + * This intentionally drops the fixed head/rest shape. + */ ? T extends Array<infer U> ? Array<ValidateSerializable<U, TSerializable>> : ReadonlyArray<ValidateSerializable<T[number], TSerializable>> : ValidateSerializableTuple<T, TSerializable>
84-122: Optionality in tuples may widen toT | undefinedDecomposing via
[infer Head, ...infer Tail]cannot preserve tuple element optionality;Head?becomesHead | undefined(required position). If this is acceptable, consider adding a type test to lock this in; if not, we can explore a mapped‑tuple approach that preserves optional flags.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (3)
packages/router-core/src/ssr/serializer/transformer.ts (3)
33-50: Solid fix: short‑circuit arrays to avoid mapped‑type recursionRouting ReadonlyArray through a dedicated path prevents infinite recursion on array props. Please confirm this precedence (array check before
T extends TSerializable) is intentional when adapters might declare array types in their serializable sets, as those would now be transformed rather than passed through.
227-236: Result typing: array short‑circuit mirrors input path (good)This aligns result validation with input, avoiding recursion for arrays and preserving readonly/mutable where applicable.
237-280: Tuple/result helpers look correct and termination-safeThe mutable/readonly tuple variants terminate and preserve tuple-ness for fixed-length tuples. Mirrors the input side consistently.
Could you add one d.ts test covering:
- a readonly variadic tuple like
readonly [number, ...string[]](documented flattening)- a tuple with optional element
[number, string?](optional vsT | undefined)
to ensure the intended behavior is locked?
brhx
force-pushed
the
fix-infinite-type
branch
3 times, most recently
from
83c533e to
669f883
Compare
brhx
force-pushed
the
fix-infinite-type
branch
from
669f883 to
b94486c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/router-core/src/ssr/serializer/transformer.ts (2)
176-186: DRY ReadableStream handling using RegisteredReadableStreamYou already define RegisteredReadableStream above; use it here to remove the duplicated mapped-type branches and improve readability.
Apply this diff:
export type ValidateSerializableResult<T, TSerializable> = T extends ReadonlyArray<unknown> ? ResolveArrayShape<T, TSerializable, 'result'> : T extends TSerializable ? T - : unknown extends SerializerExtensions['ReadableStream'] - ? { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } - : T extends SerializerExtensions['ReadableStream'] - ? ReadableStream - : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> } + : T extends RegisteredReadableStream + ? ReadableStream + : { [K in keyof T]: ValidateSerializableResult<T[K], TSerializable> }
234-238: Tighten tuple tail inference and drop redundant Readonly wrapperConstrain TTail to ReadonlyArray to improve inference and remove the need for Readonly in the recursive call.
Apply this diff:
-> = T extends readonly [infer THead, ...infer TTail] +> = T extends readonly [infer THead, ...infer TTail extends ReadonlyArray<unknown>] ? readonly [ ArrayModeResult<TMode, THead, TSerializable>, - ...ResolveTupleShape<Readonly<TTail>, TSerializable, TMode>, + ...ResolveTupleShape<TTail, TSerializable, TMode>, ] : T
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/ssr/serializer/transformer.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/ssr/serializer/transformer.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
🔇 Additional comments (1)
packages/router-core/src/ssr/serializer/transformer.ts (1)
33-49: Array/tuple-aware recursion fix looks solidDetecting ReadonlyArray first and delegating to a dedicated resolver eliminates the previous infinite recursion and correctly preserves tuple and readonly shapes. Nicely done.
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: |
|
Type performance impact btw (100 samples before and after)
Worth it imo 👌 |
2 participants
Fixes some issues I was having with https://conform.guide (also fixes #4533)
Summary by CodeRabbit
New Features
Tests