- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 53
fix: type hints for optional query and headers #218
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?
fix: type hints for optional query and headers #218
Conversation
| WalkthroughRewritten conditional typing logic for headers and query parameters in Treaty.Sign. The new evaluation order uses  Changes
 Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes involve intricate conditional type logic affecting the public API surface. Review requires careful evaluation of TypeScript type inference semantics, assessment of backwards compatibility impact on existing routes, and verification that parameter optional/required states are correctly inferred across various header and query type combinations. Poem
 Pre-merge checks and finishing touches✅ Passed checks (3 passed)
 ✨ Finishing touches
 🧪 Generate unit tests (beta)
 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ? (undefined extends Headers | ||
| ? { headers?: Record<string, unknown> } | ||
| : {} extends Headers | ||
| ? { headers?: Headers & Record<string, unknown> } | ||
| : { | ||
| headers: Headers | ||
| }) & | ||
| ({} extends Query | ||
| ? { | ||
| query?: Record<string, unknown> | ||
| } | ||
| : undefined extends Query | ||
| ? { query?: Record<string, unknown> } | ||
| (undefined extends Query | ||
| ? { query?: Record<string, unknown> } | ||
| : {} extends Query | ||
| ? { query?: Query & Record<string, unknown> } | ||
| : { query: Query }) extends infer Param | 
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.
Optional branch drops header/query schema types
When Headers (or Query) includes undefined, the new branch resolves to { headers?: Record<string, unknown> } / { query?: Record<string, unknown> }. That erases the concrete schema (e.g. 'x-token'?: string now becomes unknown), so callers lose type safety. We need to keep the original shape while still making the property optional. You can fix this by intersecting with Exclude<Headers, undefined> (and the query equivalent) so the optional case preserves the schema:
-    ? { headers?: Record<string, unknown> }
+    ? {
+          headers?: (Exclude<Headers, undefined> extends Record<string, unknown>
+              ? Exclude<Headers, undefined> & Record<string, unknown>
+              : Record<string, unknown>)
+      }and likewise for the query branch. This keeps the schema-specific property types intact while still allowing optional usage.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/treaty2/types.ts around lines 93 to 104, the optional branches for
Headers and Query currently map to Record<string, unknown> when those types
include undefined, which erases the concrete schema; change the optional
branches to preserve the original shape by intersecting with Exclude<Headers,
undefined> (and Exclude<Query, undefined> for the query branch) so the optional
property uses the original schema type (e.g. { headers?: Exclude<Headers,
undefined> & Record<string, unknown> } or equivalently an optional property
typed as Exclude<Headers, undefined>) — apply the same pattern to the query
branch to keep schema-specific keys while making the property optional.
Resolves #217
Summary by CodeRabbit