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

Type definitions for error value are unknown #483

Closed
BenJenkinson opened this issue May 12, 2020 · 15 comments
Closed

Type definitions for error value are unknown #483

BenJenkinson opened this issue May 12, 2020 · 15 comments

Comments

@BenJenkinson
Copy link

According to the docs, the error value returned by all of the hooks (useQuery, usePaginatedQuery, useInfiniteQuery, useMutation etc..) can either be null or Error.

The type definitions mostly define it as unknown or unknown | null

This should probably be considered with issue #475 since that issue suggests that the return value in the docs may be incorrect.

@thebuilder
Copy link
Contributor

So this was fixed in #514 - But I'm not sure this is the correct approach, since it expects errors to be Error objects.

This will only occur naturally by an unhandled error being thrown, and prevents you from creating custom error objects with more context. Using unknown you are forced to specify the type.

As an example, In my fetch function I'll reject the promise with the status and JSON response:

  // See if we can get a valid JSON error message from the response.
  const response = await fetch(url, request);
  const json: Result = await response.json().catch(() => {
    // Failed to parse a JSON response. Return an empty object instead.
    return {};
  });

  if (response.ok) {
    return json;
  } else {
    return Promise.reject({
      status: response.status,
      ...json,
    });
  }

I'd than handle the error by setting the type of the error:

onError: (error: SchemaProblemDetails) => {
   // In case of a 401 Unauthorized call, clear the token since it's invalid
   if (error.status === 401 && clearLogin) clearLogin();
},

This will now fail, unless I do a type conversion when reading the error value.

A better way is if we can configure the expected type of error alongside the type of data.
This is how SWR handles it.

This would have the BaseQueryOptions accept the type of error:

export interface BaseQueryOptions<Error = any> {
  /**
   * Set this to `true` to disable automatic refetching when the query mounts or changes query keys.
   * To refetch the query, use the `refetch` method returned from the `useQuery` instance.
   */
  manual?: boolean
  /**
   * If `false`, failed queries will not retry by default.
   * If `true`, failed queries will retry infinitely., failureCount: num
   * If set to an integer number, e.g. 3, failed queries will retry until the failed query count meets that number.
   * If set to a function `(failureCount, error) => boolean` failed queries will retry until the function returns false.
   */
  retry?: boolean | number | ((failureCount: number, error: Error) => boolean)
  retryDelay?: (retryAttempt: number) => number
  staleTime?: number
  cacheTime?: number
  refetchInterval?: false | number
  refetchIntervalInBackground?: boolean
  refetchOnWindowFocus?: boolean
  refetchOnMount?: boolean
  onError?: (err: Error) => void
  suspense?: boolean
  isDataEqual?: (oldData: unknown, newData: unknown) => boolean
}

And that would than have to be passed down from the hooks, so they accept the type of error in addition to the type of result, key, variables, etc.

useQuery<TResult, TKey, TError>()

@DevTGhosh
Copy link

@thebuilder I am not sure if I am doing it right but it is showing me the error Object is of type 'unknown' when I try to access error.message. Do I need to add an interface for the results of useQuery?

My code

  const { status, data, error, fetchMore, canFetchMore } = useInfiniteQuery(
    "questions",
    getStackQuestions,
    {
      getFetchMore: (lastGroup, allGroups) => lastGroup.nextPageNumber,
      staleTime: 1000 * 60 * 60 * 24 * 365,
    }
  );
  if (status === "error") {
    return <span>Error: {error.message}</span>;
  }

@thebuilder
Copy link
Contributor

You need to specify the type of the error as a generic, when creating the query.

@DevTGhosh
Copy link

@thebuilder Thank you for replying. I am not that fluent with Generics can you tell me how would that work over here? I am unable to find any good resources for it.

I see that the source file is typed like below but I am not sure how I would go about using this in my code.

// Parameter syntax with optional config
export function useInfiniteQuery<TResult = unknown, TError = unknown>(
  queryKey: QueryKey,
  queryConfig?: InfiniteQueryConfig<TResult, TError>
): InfiniteQueryResult<TResult, TError>

@thebuilder
Copy link
Contributor

You can fill out the types in side the <> after useQuery, to specify the type of your result and your errors. In this case, your errors are simply Error.

  const { status, data, error, fetchMore, canFetchMore } = useInfiniteQuery<ResultModel, Error>(
    "questions",
    getStackQuestions,
    {
      getFetchMore: (lastGroup, allGroups) => lastGroup.nextPageNumber,
      staleTime: 1000 * 60 * 60 * 24 * 365,
    }
  );
  if (status === "error") {
    return <span>Error: {error.message}</span>;
  }

@DominicTobias-b1
Copy link

DominicTobias-b1 commented Aug 30, 2020 via email

@tbntdima
Copy link

tbntdima commented Sep 9, 2020

I agree with this:

Problem is that TError comes after TResult which was
automatically inferred, you are forcing people to define both when in an
earlier version it just worked in the majority of use cases

@CloudZazu
Copy link

CloudZazu commented Sep 29, 2020

I agree with this:

Problem is that TError comes after TResult which was
automatically inferred, you are forcing people to define both when in an
earlier version it just worked in the majority of use cases

I agree as well.

It would be nice if the appropriate practice to manage this was listed in the examples.

@ptmkenny
Copy link

ptmkenny commented Feb 2, 2021

@thebuilder With your code example and react-query 3.6.0, I'm getting ResultModel is not defined. Has the name changed, or is there something else I have to do to define it?

@norbertsongin
Copy link

The most elegant solution I've found was to augment react-query module and change useMutation typings.

Here's example with formik's FormikErrors generic type:

import { FormikErrors } from 'formik';

module 'react-query' {
  export declare function useMutation<
    TData = unknown,
    TVariables = void,
    TError = FormikErrors<TVariables>,
    TContext = unknown
  >(
    mutationFn: MutationFunction<TData, TVariables>,
    options?: UseMutationOptions<TData, TError, TVariables, TContext>,
  ): UseMutationResult<TData, TError, TVariables, TContext>;
}

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 22, 2022

@norbertsongin it is not guaranteed that any errors you are getting are of a certain type. In JavaScript, anything can be thrown, so unknown is the most safe type. You can alway do an instanceof runtime check, and TypeScript will narrow the type for you.

A real life scenario for queries would be that you have a runtime error in select, which will transform your query to error state with a "normal" Error - not the type you are asserting.

This is in line with TypeScripts recent change in 4.4 to default to unknown in catch clauses.

relevant read: https://tkdodo.eu/blog/react-query-and-type-script#what-about-error

@vincerubinetti
Copy link

vincerubinetti commented Jan 12, 2023

I can kinda understand why the type should be unknown thanks to JavaScript's weirdness. Though I wonder if it could be some kind of RQ global setting? Maybe provide the type to RQ once, and assume it everywhere else? (Not sure if that's even possible in TypeScript).

Related question, if I want to manually specify Error as the error type, how would I do that while being able to keep the Data type inference from the queryFn and not have to redefine it.

useMutation<X, Error>({
    queryFn: someApiFunc
})
// where X still gets inferred automatically from someApiFunc, so I don't have to duplicate ReturnType all over my code base

Is this possible somehow? I can't find anything on Google about it.

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 13, 2023

@vincerubinetti we'll change the type to be Error per default for v5 :)

@dwilt
Copy link
Contributor

dwilt commented May 23, 2023

@TkDodo I agree with your comment that unknown is the best option here - why did you decide to go against this?

I think a win here that would help people is that inside of the onError (like onMutation for example), would be able to preserve the return type. For example:

const doSomething = useMutation({
    mutationFn: () => {}, // do something cool
    onError: (error) => {

      const _error = utilityToConvertUnknownToError({
        error,
        context: 'Failed to move a private board',
        metadata: { boards, clips, selectedBoard },
      });

      // typeof _error is now Error
      return _error;
    },
});
  
return (
  <div>
    <button onClick={doSomething.mutate}>Update</button>
    {/** Since we returned a Error from `onError`, we can safely access error.message **/}
    {doSomething.status === 'error' && <span>{error.message}}
  </div>
}

I think this still allows you to return unknown but allow the consumers to type their errors however they would like.

const doSomething = useMutation<void, Error, void>(const doSomething = useMutation({
    mutationFn: () => {}, // do something cool
    onError: (error) => {

      const _error = utilityToConvertUnknownToError({
        error,
        context: 'Failed to move a private board',
        metadata: { boards, clips, selectedBoard },
      });

      // typeof _error is now Error
      return _error;
    },
});

Passing Error as the 2nd generic to useMutation works as well but it would be nice for it to infer it.

@TkDodo
Copy link
Collaborator

TkDodo commented May 27, 2023

@TkDodo I agree with #483 (comment) that unknown is the best option here - why did you decide to go against this?

Balancing type safety with ergonomics, mostly. Defaulting to Error seems like it's what you'd have 99.5% of the time, and it's in-line with JS best practices to only throw Errors and nothing else. It will allow us to access error.message without having to check for it first.

Also, the new Register interface in v5 allows you to easily go back to unknown if you prefer that:

https://tanstack.com/query/v5/docs/react/typescript#registering-a-global-error

declare module '@tanstack/react-query' {
  interface Register {
    defaultError: unknown
  }
}

the onXXX callbacks are callbacks to execute side-effects, not to transform data. You also can't transform data in onSuccess with it. What you can do from those callbacks is return a Promise, which will be awaited internally:

onError: () => {
  return queryClient.invalidateQueries()
}

this makes sure your mutation will stay in pending state while the invalidation is ongoing. So I don't think your proposal is doable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests