Skip to content
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

Feature Request: Create new ts-rest client that throws exceptions for error responses #520

Closed
toteto opened this issue Mar 21, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@toteto
Copy link
Contributor

toteto commented Mar 21, 2024

We are currently utilizing the ts-rest library on both the client and server side of our application. The client side is built with React and uses react-query. However, we are not using the @ts-rest/react-query client due to compatibility issues with our usage patterns.

Our previous experience with Zodios and Axios for executing requests has led us to observe that the resolution of all responses (including non-success ones) can result in weird code flow. This issue is also observable in the provided examples.

Example

import { useQuery } from "@tanstack/react-query";

function App() {
  const { mutation, isPending, isError, isSuccess } = useMutation({
    mutationFn: (newPost) => client.addPost(newPost),
    onSuccess: (response) => {
        if (response.status === 200) // do success
        if (response.status === 500) // do error stuff
    },
    onError: (error) => {
        // do other error stuff
    }
  });

  return <div>
    <h1>Posts</h1>
    <div>{/** render posts */}</div>
    <button onClick={mutation}>Save post</button>
  </div>
}

The above example presents several issues:

  • Upon execution of the mutation, all responses are directed to the onSuccess callback. This includes error paths, which is not ideal. Error-handling should not be performed in the mutationFn, as it results in a non-ideal response type (if we return undefined in the mutationFn, the type of response will be Response | undefined).
  • The onError callback handles errors for the second time.
  • The isError/isSuccess flags are incorrect, as an error response can be considered a success.
  • A similar issue exists for error response handling in client router data loaders. The data loaders also have error handler callbacks, and in most cases, throwing the error makes error handling easier.

Proposal

To address these issues, we propose the following:

  • Develop a client that treats responses in the 2xx range as successful and resolves the promise. In most cases, there will be only a single 2xx response definition in the contract, simplifying success handling (you don't have to handle the error case to handle the success case).
  • Responses outside of the success range should be thrown as errors. Errors should be thrown with an error object containing the error response.
  • Implement a type-guard method that checks if a given error is thrown by a particular endpoint, isTsRestError(route: AppRoute | AppRouter, status : ErrorHttpStatus, error: Error) : error is ...
  • (Optional) Create a configuration that marks particular responses as success or error. This could potentially be achieved via the contract definition ({ status: 200, body: ...}, { status: 201, isError: true, body: ... }). This could be a router configuration or client configuration similar to the axios.validateStatus function: axios-http.com/docs/handling_errors

Example from above with

import { useQuery } from "@tanstack/react-query";
function App() {
  const { mutation, isPending, isError, isSuccess } = useMutation({
    mutationFn: (newPost) => client.addPost(newPost),
    onSuccess: (response) => {
       // response.status is only 200
       // do optimistic update, do success
    },
    onError: (error) => {
        if (isTsError(client.addPost.route, 500, error)) // do error stuff
        // do other error stuff
    }
  });

  return <div>
    <h1>Posts</h1>
    <div>{/** render posts */}</div>
    <button onClick={mutation}>Save post</button>
  </div>
}
  • The success path is very clear, it will be response.body in most cases because there will be only one success status
  • The error path is clear, all kinds of error are handlded in a single place. The type safe response remain.
  • The isError/isSuccess flags are correct
  • Error can be handled anywhere in the application in type safe manner, it can even be handled in the root error handler.
@Gabrola
Copy link
Member

Gabrola commented Mar 22, 2024

Thank you for your proposal! Pretty well thought out, but you've overlooked a major point, which is why @ts-rest/react-query exists in the first place, and which solves this entire use-case, which I'll elaborate on later.

The thing about ts-rest is that we try to make its usage as convenient as possible while being as unopinionated as much as possible. I believe we have struck this right balance.

Our core client uses fetch which does not throw on error response codes, therefore we do not. fetch is the standard now on the web, so we are sticking to its conventions. It's not really in our philosophy to be creating custom clients ourselves with elaborate error handling or features such as middleware, or auth or whatever since there are many different use-cases that we cannot cover all, or some, without being opinionated.

A such, we have provided functionality in order to allow everyone to customize the "client" to behave however they want it, or even swap out fetch entirely if they want to. This is the only way we can guarantee that ts-rest works for everyone.

In your case you can just simply wrap the tsRestFetcher and do the error handling in a custom fetcher.

const client = initQueryClient(postsApi, {
  baseUrl: 'http://localhost:5003',
  baseHeaders: {},
  api: async (args) => {
    const response = await tsRestFetchApi(args);
    if (!String(result.status).startsWith('2')) {
      throw response;
      // or wrap with your own error, whatever
    }
    return response;
  },
});

Now for react query, @ts-rest/react-query mainly exists as a typing wrapper rather than providing extra runtime functionality. If you opt to use just react-query then you will need to do lots of manual typing work regardless of if you use your own custom fetcher or if we theoretically used a newly proposed ts-rest client.

const { mutate } = useMutation<
  ClientInferResponses<typeof contract.addPost, SuccessfulHttpStatusCode, 'force'>,
  ClientInferResponses<typeof contract.addPost, ErrorHttpStatusCode, 'ignore'> // or whatever you wrap the error with
>({
  mutationFn: (newPost) => client.addPost(newPost),
  onSuccess: (response) => {
  },
  onError: (error) => {
    // or type guard here instead of using the type generics
  },
});

For the error typing you will still have to either manually type useMutation or useQuery's second generic or type guard the error every single time. It will get very repetitive.

Anyway it seems that your problem with using @ts-rest/react-query is something very specific and it's hard to justify building extra functionality that will take development effort to maintain just to cover a very specific use-case.

However, the types and functions to assist with building this functionality yourself is already there.

@toteto
Copy link
Contributor Author

toteto commented Mar 28, 2024

@Gabrola
Thank you for your prompt response, and I apologize for the delay in my reply. I missed the notification in my inbox.

Regarding my usage of @ts-rest/react-query, I initially misunderstood an implementation detail: this client actually only allows 2xx responses in the onSuccess callback. This behavior aligns with my requirements for a standalone client.

While @ts-rest/react-query generally meets my needs, there are specific cases where I encounter challenges. For instance, in a single useMutation.mutationFn, I sometimes need to perform multiple steps during the mutation process. Additionally, the error type in orError is currently incorrect; it should handle any thrown errors, not just error status responses.

Putting aside my personal preferences and specific use case, I wonder if there's a way to engage the community and discuss the desired behavior of the core client.

One observation that leads me to believe that throwing errors might be more desirable is the implementation in @ts-rest/react-query. Furthermore, many developers prefer the API of axios over fetch.

I’d appreciate any insights or thoughts on PR #522 whenever you have a moment.

As a temporary solution, I've created a utility function in my code that can be chained to the response promise. This utility returns the success response but throws an error response when necessary:

export function successBody<TStatus extends SuccessfulHttpStatusCode, TRes extends Res>(
    response: TRes,
    status?: TStatus,
): (TRes & { status: TStatus extends undefined ? SuccessfulHttpStatusCode : TStatus })['body'] {
    if (status != null ? response.status === status : response.status >= 200 && response.status < 300)
        return response.body;
    throw new TsRestError(response);
}

const { status, body: posts } = client.getPosts({...}).then(successBody);
// status is 2xx, posts is of type Post[]
// Wrap in a try-catch block to handle errors with status codes outside the 2xx range

However, I'm not particularly fond of this approach, as I need to remember to include it in every method call.

@Gabrola
Copy link
Member

Gabrola commented Apr 2, 2024

@toteto I left you some comments on the PR. However, I still don't believe much in the utility of throwing on fetch except to trigger an error in tanstack query. Otherwise, it's pretty much the same amount of code; whether you wrap in try/catch, or if you just create an if condition to check for the success status. This leads me to believe, that this change only exists to cover the use-case of wanting to use tanstack query but not wanting to use the ts-rest wrappers.

Even if you were to manually use tanstack query, all of the new types in the regular client are still useless because you still have to manually type the error object, and since the error type is the second generic on useQuery, etc. you will still have to manually pass the success type and error type every single time you use useQuery or useMutation.

Regarding not using @ts-rest/react-query, if you need to perform any steps before running a mutation, then simply just run these steps before calling mutate.

I cannot justify including code for a feature, just to cover a very specific use-case that has not been requested by anyone else yet.

@Gabrola
Copy link
Member

Gabrola commented Apr 2, 2024

And yes, regarding the incorrectly typed error object in @ts-reset/react-query, I am aware of that, however it is too late to fix now without a breaking change. So we'll be fixing it in v4

@ivan-kleshnin
Copy link
Contributor

ivan-kleshnin commented Apr 4, 2024

@Gabrola is it possible to use a client created with initQueryClient for direct (hook-less) fetches? I mean:

const client = initQueryClient(...)

client.getPost.useQuery() // -- does work
// then, at some place in code, I need raw `client.getPost()`
await client.getPost() // -- is not a function, any alternative syntax?

Should I create 2 clients: one for higher- and another for lower-level API handling?
Will they have a single shared cache in that case?

I use React-Query but it's often necessary to make direct calls, without useQuery wrapper – be it a server-side prefetchQuery() or another necessity.

@ivan-kleshnin
Copy link
Contributor

ivan-kleshnin commented Apr 4, 2024

@toteto yeah, familiar situation 😅 . IMO it's easier to remap your understanding of "errors" than to fight with tooling.

onError corresponds to code/client error.
onSuccess corresponds to the absence of the above errors.

From the client point of view 404, 500, etc. response status codes are not errors but expected scenarios. So you handle them in the same branch as 200.

If you think about it like this, then everything behaves as expected.

@Gabrola
Copy link
Member

Gabrola commented Apr 4, 2024

@ivan-kleshnin there are .query() and .mutation() functions exposed that will fetch directly. So you can do await client.getPost.query()

@Gabrola Gabrola added the enhancement New feature or request label Apr 15, 2024
@monopolo11
Copy link

monopolo11 commented May 17, 2024

So we'll be fixing it in v4

Hi @Gabrola

Is there an ETA on v4?

Or is there some way we can try and contribute to it's development?

@toteto toteto closed this as completed May 23, 2024
@toteto toteto closed this as not planned Won't fix, can't repro, duplicate, stale May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants