feat: ts-rest client throw on error status code #522
Conversation
|
@toteto is attempting to deploy a commit to the ts-rest Team on Vercel. A member of the Team first needs to authorize it. |
|
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 5884d86. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 4 targetsSent with 💌 from NxCloud. |
|
|
||
| export type FetchOptions = Omit<RequestInit, 'method' | 'headers' | 'body'>; | ||
|
|
||
| export interface OverrideableClientArgs { | ||
| baseUrl: string; | ||
| credentials?: RequestCredentials; | ||
| jsonQuery?: boolean; | ||
| throwOnErrorStatus?: boolean; |
There was a problem hiding this comment.
This option can be confusing because it will be available to the rest of the tanstack clients, however we don't/cannot really throw there. However, it actually needs to be available for all clients, because we still expose the vanilla .query() and .mutation() functions to these clients.
| args?: Prettify<TArgs>, | ||
| ) => Promise< | ||
| Prettify< | ||
| TClientArgs['throwOnErrorStatus'] extends true |
There was a problem hiding this comment.
This will be incorrectly typed if the throwOnErrorStatus was overriden. Need to check if it was overriden from TArgs.
| (clientArgs.throwOnErrorStatus && response.status < 200) || | ||
| response.status >= 300 |
| TAppRoute, | ||
| TErrorStatus extends undefined ? ErrorHttpStatusCode : TErrorStatus | ||
| > { | ||
| if (error == null) return false; |
There was a problem hiding this comment.
We don't use non-strict equality in our code base. In addition, you don't need to check for falsy values before using instanceof
| TErrorStatus extends undefined ? ErrorHttpStatusCode : TErrorStatus | ||
| > { | ||
| if (error == null) return false; | ||
| if (!(error instanceof ErrorStatusResponseError)) return false; |
There was a problem hiding this comment.
We don't use shorthand single-line ifs in our codebase.
|
|
||
| if (errorStatusResponseError.route.path !== route.path) return false; | ||
|
|
||
| if (status == null) return true; |
There was a problem hiding this comment.
Optional args are undefined, not null. This only works because of the non-strict equality. Should be
| if (status == null) return true; | |
| if (status === undefined) return true; |
But just easier to do if (!status) { return true; } and easier to read
| constructor( | ||
| public readonly route: TAppRoute, | ||
| public readonly response: ErrorResponse<TAppRoute, TErrorStatus>, | ||
| public readonly request: unknown, |
There was a problem hiding this comment.
Can't this be ClientInferRequest<TAppRoute, ClientArgs>?
| ErrorHttpStatusCode | ||
| >; | ||
|
|
||
| if (errorStatusResponseError.route.path !== route.path) return false; |
There was a problem hiding this comment.
Need to check the method too, not only the path.
| [401, 403, 500], | ||
| ); | ||
|
|
||
| if (!isHealthError) fail('should be health error'); |
There was a problem hiding this comment.
Use expect(isHealthError).toBe(true)
| status?: TErrorStatus | TErrorStatus[], | ||
| ): error is ErrorStatusResponseError< | ||
| TAppRoute, | ||
| TErrorStatus extends undefined ? ErrorHttpStatusCode : TErrorStatus |
There was a problem hiding this comment.
TErrorStatus extends undefined will never be true because you already declared it as TErrorStatus extends ErrorHttpStatusCode. Even if not, T extends undefined in general doesn't work that way for optional parameters, because they resolve as unknown not undefined, and actually because extends is distributive you need to do unknown extends T not T extends unknown
This should work fine if you just replace TErrorStatus extends undefined ? ErrorHttpStatusCode : TErrorStatus with TErrorStatus because you already gave it a default value of ErrorHttpStatusCode, so it will take that if you don't pass status
|
Closing in favour of having un-opinionated implementation |
Resolves #520
There are no breaking changes with this new addition. In order to get this behavior of the client to throw on error status code, a parameter of
throwOnErrorStatus: trueneeds to be provided to theinitClientoptions.Open for changes or feedback to the code. If you don't see this as an addition of the
@ts-rest/coreclient, looking for suggestion how such behavior can be achieved by separate client that can be in separate package/repository.