feat(client, server): throw if plugin context is missing #291
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request simplifies generic type constraints in three plugin files. In the client plugin, the Changes
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
More templates
@orpc/arktype
@orpc/client
@orpc/openapi
@orpc/openapi-client
@orpc/contract
@orpc/react-query
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/svelte-query
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/client/src/plugins/retry.ts(2 hunks)packages/server/src/plugins/cors.ts(2 hunks)packages/server/src/plugins/response-headers.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/client/src/plugins/retry.ts (1)
packages/client/src/plugins/base.ts (1)
ClientPlugin(4-6)
packages/server/src/plugins/response-headers.ts (2)
packages/server/src/plugins/base.ts (1)
HandlerPlugin(4-6)packages/server/src/adapters/standard/handler.ts (1)
StandardHandlerOptions(24-46)
packages/server/src/plugins/cors.ts (2)
packages/server/src/adapters/standard/handler.ts (2)
StandardHandlerInterceptorOptions(20-22)StandardHandlerOptions(24-46)packages/server/src/plugins/base.ts (1)
HandlerPlugin(4-6)
🔇 Additional comments (2)
packages/client/src/plugins/retry.ts (1)
3-3:❓ Verification inconclusive
Import removed but potentially needed for ClientPlugin compatibility.
The import for
ClientContexthas been removed, but this might be problematic since:
- The
ClientPlugininterface requires typeT extends ClientContext- Without explicitly extending
ClientContext, the plugin might not be compatible with its base interface
🏁 Script executed:
#!/bin/bash # Verify if ClientRetryPluginContext extends ClientContext which would validate this change echo "Checking if ClientRetryPluginContext extends ClientContext..." rg -A 3 "interface\s+ClientRetryPluginContext\s+extends\s+ClientContext" packages/client/src/ # Also check the ClientPlugin interface definition to confirm the requirement echo "Checking ClientPlugin interface definition..." rg -A 3 "interface\s+ClientPlugin" packages/client/src/plugins/base.tsLength of output: 509
Action Required: Verify Compatibility of Plugin Context
The
ClientPlugininterface (seepackages/client/src/plugins/base.ts) requires its generic type to extendClientContext. Inpackages/client/src/plugins/retry.ts, the only imported type isClientOptionsOut, and the explicit import forClientContexthas been removed. However, our verification did not find any declaration showing that the retry plugin’s context (e.g. a potentialClientRetryPluginContextorClientOptionsOut) explicitly extendsClientContext.Please verify that:
- The type passed to
ClientPlugin(likelyClientOptionsOut) either directly extends or is compatible withClientContext.- If it does not, the
ClientContextimport should be reintroduced and the context type adjusted accordingly to ensure compatibility with the baseClientPlugininterface.packages/server/src/plugins/cors.ts (1)
7-9: Generic type parameter renamed for consistency.The generic type parameter has been renamed from
TContexttoTthroughout the file while maintaining the same type constraintextends Context. This change improves naming consistency across plugins while preserving type safety.Also applies to: 17-21, 32-32
| export class ResponseHeadersPlugin<T extends ResponseHeadersPluginContext> implements HandlerPlugin<T> { | ||
| init(options: StandardHandlerOptions<T>): void { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type constraint is simplified but may have compatibility concerns.
The generic type parameter has been renamed from TContext to T and the type constraint has been simplified to only extend ResponseHeadersPluginContext instead of both ResponseHeadersPluginContext & Context. This change might cause type incompatibility since:
HandlerPlugin<T>(which this class implements) requiresT extends Contextaccording to the base interface inplugins/base.tsStandardHandlerOptions<T>in theinitmethod requiresT extends Context
🏁 Script executed:
#!/bin/bash
# Check if ResponseHeadersPluginContext extends Context to verify if the simplified type constraint is sufficient
# First, search if ResponseHeadersPluginContext extends Context directly
echo "Checking if ResponseHeadersPluginContext extends Context directly..."
rg -A 3 "interface\s+ResponseHeadersPluginContext\s+extends\s+Context" packages/server/src/
# Check if ResponseHeadersPluginContext is potentially part of a type hierarchy that extends Context
echo "Checking type hierarchy..."
rg -A 5 "ResponseHeadersPluginContext" packages/server/src/Length of output: 2428
Let's further verify whether the base interface imposes a T extends Context constraint. We'll search for "interface HandlerPlugin" in the repository to see the actual declaration.
Once we confirm that, we can finalize our response.
🏁 Script executed:
#!/bin/bash
rg -A 3 "interface\s+HandlerPlugin<" -n .Length of output: 322
Action Required: Resolve Generic Type Compatibility Issue
The current change restricts T to extend ResponseHeadersPluginContext, but the base interface (HandlerPlugin<TContext extends Context>) and StandardHandlerOptions<T> require T to extend Context. Since ResponseHeadersPluginContext does not extend Context, this simplified constraint may lead to type incompatibility. Consider one of the following fixes:
- Update
ResponseHeadersPluginContextto extendContext(e.g.,export interface ResponseHeadersPluginContext extends Context { ... }), or - Revise the generic constraint in
ResponseHeadersPluginto enforce both requirements (e.g., change the declaration toT extends ResponseHeadersPluginContext & Context).
Addressing this will ensure that implementations correctly satisfy all expected type constraints across the codebase.
| @@ -62,7 +62,7 @@ export interface ClientRetryPluginOptions { | |||
| default?: ClientRetryPluginContext | |||
| } | |||
|
|
|||
| export class ClientRetryPlugin<T extends ClientContext & ClientRetryPluginContext> implements ClientPlugin<T> { | |||
| export class ClientRetryPlugin<T extends ClientRetryPluginContext> implements ClientPlugin<T> { | |||
There was a problem hiding this comment.
Type constraint is simplified but may break interface compatibility.
The type constraint has been simplified from T extends ClientContext & ClientRetryPluginContext to just T extends ClientRetryPluginContext. This could cause compatibility issues because:
ClientPlugin<T>(which this class implements) requiresT extends ClientContext- Without the
ClientContextconstraint, the type system might not enforce required properties expected by the core client code
Consider maintaining the ClientContext constraint or ensure that ClientRetryPluginContext extends ClientContext:
-export class ClientRetryPlugin<T extends ClientRetryPluginContext> implements ClientPlugin<T> {
+export class ClientRetryPlugin<T extends ClientContext & ClientRetryPluginContext> implements ClientPlugin<T> {Alternative solution if ClientRetryPluginContext should extend ClientContext:
export interface ClientRetryPluginContext extends ClientContext {
// existing properties...
}
Summary by CodeRabbit