feat(types): add schema-typed ofetch.create with OpenAPI-style path inference (#406)#548
Open
feat(types): add schema-typed ofetch.create with OpenAPI-style path inference (#406)#548
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds schema-driven TypeScript typing to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/types.test.ts (1)
4-81: Use a fixture that matches the public OpenAPI contract.This only exercises
createFetch<ApiPaths>()with static paths and method-local shapes, so it won’t catch the cases this PR is advertising:ofetch.create<paths>(), templated routes, sharedpaths["..."]["parameters"],defaulterror responses, or 204/no-content responses. Add at least one realistic"/users/{id}"case through.create<paths>()so the intended integration is actually pinned down. (openapi-ts.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/types.test.ts` around lines 4 - 81, The test fixture ApiPaths used by createFetch in the typed tests is too minimal and misses templated routes and shared parameter/response cases; update the ApiPaths type and the test calls to include a realistic templated route such as "/users/{id}" with path parameters and multiple response shapes (e.g., 200 JSON user, 204 no-content, default error response), exercise createFetch via createFetch<ApiPaths> (the typedFetch constant) and add calls that use templated route invocation, path parameter typing, and default/error responses so the test covers ofetch.create<paths>(), templated routes, shared paths["..."]["parameters"], and 204/no-content behavior referenced in the review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types.ts`:
- Around line 176-179: The declaration for create<TNewSchema = TSchema>
currently requires defaults, causing valid runtime calls like ofetch.create() to
type-error; update the signature in src/types.ts so defaults is optional (or has
a default empty object) e.g. make the first parameter optional (defaults?:
FetchOptions) or defaults: FetchOptions | undefined to match src/fetch.ts which
implements create(defaultOptions = {}, customGlobalOptions = {}), keeping
globalOptions (CreateFetchOptions) optional and preserving the generic
$Fetch<TNewSchema> return type.
- Around line 89-95: ResponseBodyFromResponse currently returns never when a
success response has no content, causing endpoints like 204 to type as
Promise<never>; update ResponseBodyFromResponse so that if T has no content or
its content type has no keys (keyof TContent is never) it resolves to undefined,
otherwise return TContent[keyof TContent]; keep OperationData<T> as
ResponseBodyFromResponse<SuccessResponses<OperationResponses<T>>> so callers get
undefined for contentless 2xx responses instead of never.
- Around line 78-87: SuccessStatusCode wrongly includes "default" causing
SuccessResponses to include the default error branch; update the types so
"default" is excluded from successful status resolution. Concretely, modify
SuccessStatusCode (or the mapping in SuccessResponses) to only recognize 2xx
codes and "2XX" (remove "default") or add an explicit conditional in
SuccessResponses to treat TStatus extends "default" ? never : ... so that keys
named "default" are never included in the resolved success payload; ensure the
symbols to change are SuccessStatusCode and/or the SuccessResponses mapped type.
- Around line 97-124: OperationQuery only extracts query params and
TypedFetchOptions lacks any slot for path parameters, so add a path-parameters
extraction and include it in the options: introduce a new type (e.g.,
OperationPath<T>) that mirrors OperationQuery but extracts parameters with "in:
'path'" (or rework OperationQuery to return a union of query and path objects),
create a PathOption<T> (similar to QueryOption<T>) that exposes a typed
pathParams?: OperationPath<T> (or required body-style when path params are
required), and extend TypedFetchOptions<T, R> to intersect PathOption<T> so
callers can supply path parameter values; also update any request invocation
type (the function that accepts the literal path) to accept a templated path +
pathParams or a function overload that accepts a path template string and a
typed params object, referencing OperationQuery, QueryOption, BodyOption,
TypedFetchOptions, OperationRequestBody, OperationHasRequiredBody, FetchOptions,
and OperationData to locate the relevant types to modify.
---
Nitpick comments:
In `@test/types.test.ts`:
- Around line 4-81: The test fixture ApiPaths used by createFetch in the typed
tests is too minimal and misses templated routes and shared parameter/response
cases; update the ApiPaths type and the test calls to include a realistic
templated route such as "/users/{id}" with path parameters and multiple response
shapes (e.g., 200 JSON user, 204 no-content, default error response), exercise
createFetch via createFetch<ApiPaths> (the typedFetch constant) and add calls
that use templated route invocation, path parameter typing, and default/error
responses so the test covers ofetch.create<paths>(), templated routes, shared
paths["..."]["parameters"], and 204/no-content behavior referenced in the review
comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e94f54a9-2be0-47dc-93e0-45917e49c676
📒 Files selected for processing (4)
README.mdsrc/fetch.tssrc/types.tstest/types.test.ts
Comment on lines
+78
to
+87
| type SuccessStatusCode = `${2}${number}${number}` | "2XX" | "default"; | ||
|
|
||
| type SuccessResponses<TResponses> = | ||
| TResponses extends Record<string, any> | ||
| ? { | ||
| [TStatus in keyof TResponses]: `${TStatus & (string | number)}` extends SuccessStatusCode | ||
| ? TResponses[TStatus] | ||
| : never; | ||
| }[keyof TResponses] | ||
| : never; |
There was a problem hiding this comment.
Don’t fold default into the resolved success payload.
SuccessStatusCode currently treats default like a 2xx branch, so a schema like { 200: User; default: ApiError } resolves as Promise<User | ApiError>. Since src/fetch.ts throws on 4xx/5xx unless ignoreResponseError is set, that makes the normal success type too wide.
Suggested diff
-type SuccessStatusCode = `${2}${number}${number}` | "2XX" | "default";
+type SuccessStatusCode = `${2}${number}${number}` | "2XX";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types.ts` around lines 78 - 87, SuccessStatusCode wrongly includes
"default" causing SuccessResponses to include the default error branch; update
the types so "default" is excluded from successful status resolution.
Concretely, modify SuccessStatusCode (or the mapping in SuccessResponses) to
only recognize 2xx codes and "2XX" (remove "default") or add an explicit
conditional in SuccessResponses to treat TStatus extends "default" ? never : ...
so that keys named "default" are never included in the resolved success payload;
ensure the symbols to change are SuccessStatusCode and/or the SuccessResponses
mapped type.
Comment on lines
+89
to
+95
| type ResponseBodyFromResponse<T> = T extends { content: infer TContent } | ||
| ? TContent[keyof TContent] | ||
| : never; | ||
|
|
||
| type OperationData<T> = ResponseBodyFromResponse< | ||
| SuccessResponses<OperationResponses<T>> | ||
| >; |
There was a problem hiding this comment.
Return undefined for contentless 2xx responses.
If a success response omits content, ResponseBodyFromResponse becomes never, so common 204 endpoints type as Promise<never> even though runtime resolves with _data === undefined.
Suggested diff
type ResponseBodyFromResponse<T> = T extends { content: infer TContent }
? TContent[keyof TContent]
- : never;
+ : undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type ResponseBodyFromResponse<T> = T extends { content: infer TContent } | |
| ? TContent[keyof TContent] | |
| : never; | |
| type OperationData<T> = ResponseBodyFromResponse< | |
| SuccessResponses<OperationResponses<T>> | |
| >; | |
| type ResponseBodyFromResponse<T> = T extends { content: infer TContent } | |
| ? TContent[keyof TContent] | |
| : undefined; | |
| type OperationData<T> = ResponseBodyFromResponse< | |
| SuccessResponses<OperationResponses<T>> | |
| >; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types.ts` around lines 89 - 95, ResponseBodyFromResponse currently
returns never when a success response has no content, causing endpoints like 204
to type as Promise<never>; update ResponseBodyFromResponse so that if T has no
content or its content type has no keys (keyof TContent is never) it resolves to
undefined, otherwise return TContent[keyof TContent]; keep OperationData<T> as
ResponseBodyFromResponse<SuccessResponses<OperationResponses<T>>> so callers get
undefined for contentless 2xx responses instead of never.
Comment on lines
+176
to
+179
| create<TNewSchema = TSchema>( | ||
| defaults: FetchOptions, | ||
| globalOptions?: CreateFetchOptions | ||
| ): $Fetch<TNewSchema>; |
There was a problem hiding this comment.
Keep .create() callable without arguments.
Runtime still supports ofetch.create() because src/fetch.ts implements create(defaultOptions = {}, customGlobalOptions = {}). Making defaults mandatory here turns an existing valid call into a type error with no behavior change.
Suggested diff
create<TNewSchema = TSchema>(
- defaults: FetchOptions,
+ defaults?: FetchOptions,
globalOptions?: CreateFetchOptions
): $Fetch<TNewSchema>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types.ts` around lines 176 - 179, The declaration for create<TNewSchema =
TSchema> currently requires defaults, causing valid runtime calls like
ofetch.create() to type-error; update the signature in src/types.ts so defaults
is optional (or has a default empty object) e.g. make the first parameter
optional (defaults?: FetchOptions) or defaults: FetchOptions | undefined to
match src/fetch.ts which implements create(defaultOptions = {},
customGlobalOptions = {}), keeping globalOptions (CreateFetchOptions) optional
and preserving the generic $Fetch<TNewSchema> return type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #406
Adds type support for passing OpenAPI-style schema types to ofetch.create (for example from openapi-typescript), enabling typed path/method/body/response inference.
Changes:
Summary by CodeRabbit
New Features
Documentation
Tests