-
-
Notifications
You must be signed in to change notification settings - Fork 69
feat(rpc): runtime type validation for rpc calls #161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
commit: |
|
I think this is mostly done. Before adding some tests, I wanted to check whether I’m heading in the right direction. Since the definitions of RPC methods are scattered across multiple packages ( This approach, however, introduces some additional exports and also creates cyclic dependencies. As a result, I worked around this by marking To avoid the cyclic dependency, it looks like one option would be to move all schemas into I might be missing something here, so I’d really appreciate some guidance on the intended architecture. In particular, I’d love to understand where you expect the schemas to live and how the dependency direction between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds runtime type validation to RPC calls by integrating valibot for schema-based validation. The changes introduce schema definitions for RPC functions and validate both arguments and return values on the client side.
Changes:
- Added valibot dependency and schema definitions for RPC functions
- Created validation utilities for runtime type checking of RPC arguments and return values
- Exported RPC schemas from core and vite packages for use in validation
- Fixed build configuration to externalize lightningcss dependency
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Added valibot v1.2.0 to dependency catalog |
| pnpm-lock.yaml | Lock file update for valibot dependency |
| packages/kit/package.json | Added valibot as a dependency to the kit package |
| packages/kit/src/utils/define.ts | Enhanced defineRpcFunction to support optional schema definitions for args and return values |
| packages/kit/src/utils/rpc-validation.ts | New validation utility implementing runtime type checking for RPC calls |
| packages/kit/src/client/rpc.ts | Integrated validation into the RPC call method |
| packages/core/src/node/rpc/index.ts | Added builtinRpcSchemas export containing schema mappings |
| packages/core/src/index.ts | Exported builtinRpcSchemas for external use |
| packages/vite/src/node/rpc/index.ts | Added serverRpcSchemas export containing schema mappings |
| packages/vite/src/node/index.ts | Exported serverRpcSchemas for external use |
| packages/core/src/node/rpc/internal/docks-on-launch.ts | Example implementation adding schema definitions to an RPC function |
| packages/core/src/node/cli-commands.ts | Refactored to use rpc.definitions instead of rpc.functions |
| packages/vite/tsdown.config.ts | Added lightningcss to external dependencies |
| packages/vite/src/nuxt.config.ts | Added lightningcss to Rolldown external dependencies |
| test/exports/*.yaml | Updated export snapshots with new schema exports |
| packages/core/playground/vite.config.ts | Added debug dock entry to test RPC calls |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,3 +1,4 @@ | |||
| export { builtinRpcSchemas } from '../../core/src/node/rpc' | |||
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import path is incorrect. It should be './node/rpc' instead of '../../core/src/node/rpc'. The current path goes up two directories to packages/, then back down to core/src/node/rpc, which is unnecessarily complex and could cause issues if the directory structure changes.
| export { builtinRpcSchemas } from '../../core/src/node/rpc' | |
| export { builtinRpcSchemas } from './node/rpc' |
| const schema = getSchema(method) | ||
| if (!schema) | ||
| throw new Error(`RPC method "${method}" is not defined.`) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema existence check on lines 29-30 is redundant because getSchema() already throws an error if the schema is not found (lines 22-23). This duplicate check can be removed.
| call: (...args: any): any => { | ||
| return serverRpc.$call( | ||
| validateRpcArgs(args[0], args.slice(1)) | ||
| const ret = serverRpc.$call( | ||
| // @ts-expect-error casting | ||
| ...args, | ||
| ) | ||
| validateRpcReturn(args[0], ret) | ||
| }, |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call method has three issues: (1) Missing return statement - the function should return 'ret', (2) The validation of return value should happen after the promise resolves since $call returns a Promise, requiring async/await or .then(), (3) The function signature should be async if validating async return values. Consider: call: async (...args: any): Promise<any> => { validateRpcArgs(args[0], args.slice(1)); const ret = await serverRpc.$call(...args); validateRpcReturn(args[0], ret); return ret; }
| export function validateRpcReturn(method: string, data: any) { | ||
| const schema = getSchema(method) | ||
| if (!schema) | ||
| throw new Error(`RPC method "${method}" is not defined.`) |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema existence check on lines 49-50 is redundant because getSchema() already throws an error if the schema is not found (lines 22-23). This duplicate check can be removed.
|
|
||
| declare module '@vitejs/devtools-kit' { | ||
| export interface DevToolsRpcServerFunctions extends BuiltinServerFunctions {} | ||
| export interface DevToolsRpcServerFunctions extends BuiltinServerFunctions { } |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an unnecessary space before the closing brace. While this doesn't affect functionality, it's inconsistent with TypeScript formatting conventions. Should be extends BuiltinServerFunctions {}
| export interface DevToolsRpcServerFunctions extends BuiltinServerFunctions { } | |
| export interface DevToolsRpcServerFunctions extends BuiltinServerFunctions {} |
|
Thanks! But I think it might be better to do it in |
Description
Introduce valibot to rpc interface, for better type generation and API verification.
Linked Issues
#9
Additional context