apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.32k stars 2.65k forks source link

Errors lost on cached results #4644

Open joepuzzo opened 5 years ago

joepuzzo commented 5 years ago

I'm using apollo and react to call a query that returns mixed results: error(s) and some data. In order to handle the errors, but still be able to use the information given, I'm using the errorPolicy: "all" option in the query.

<Query query={query} variables={variables as any} errorPolicy='all'>
    {({loading, error, data}) => { ... }}
</Query>

The first time I mount the component data is populated with the partial informations and error with the errors returned by the query. If I change the route (unmounting the component) and then return on it, it shows the cached partial data but no errors, so I'm not able to handle errors anymore and detect that these are partial informations.

Intended outcome: The component shows me the original errors along with the cached data.

Actual outcome: The props error is undefined, the partial data are passed as if the query didn't return any error.

How to reproduce the issue: Create a query that returns both data and error.

Version

"react-apollo": "2.5.2", "apollo-client": "^2.5.1",

Reference to docs:

Screen Shot 2019-03-27 at 8 43 37 AM

From the docs ^^^^^

https://www.apollographql.com/docs/react/features/error-handling

If this issue looks familiar thats because i stole part of the description from here. An issue that was previously opened and then closed.

Im not sure why that issue was closed as a solution to the original issue was never found.

Note: I am NOT using SSR, i'm simply using the in meme cache.

hwillson commented 5 years ago

@joepuzzo Any chance you could create a small runnable reproduction that demonstrates this happening?

joepuzzo commented 5 years ago

I will try to throw a code sandbox together and get back to ya!

joepuzzo commented 5 years ago

@hwillson Ok ive got two sandboxes for ya!

https://codesandbox.io/s/w7qy5m1pw7 << UI using apollo client https://codesandbox.io/s/5wlv3jlywn << Server using apollo server

A couple things to note:

  1. The following parameters seem to have NO affect on the policies.. you MUST put the param on the <Query> component.
defaultOptions: {
    watchQuery: {
      errorPolicy: "all"
    },
    query: {
      errorPolicy: "all"
    },
    mutate: {
      errorPolicy: "all"
    }
  }
  1. Putting errorPolicy="all" on the query makes it so the cacheing takes place ( the request is made every time other wise (i find this to be incorrect).
joepuzzo commented 5 years ago

To reproduce: Go to the home page and refresh the browser. Then navigate to the dog page. You will see an error. Then navigate back to home and then back again to dog the error was not cached and there was obviously no re attempt to get the data again <<< which would also be a nice feature.

t3dotgg commented 5 years ago

I think this might be related to another bug in react-apollo around the onError prop not firing when errorPolicy='all'.

I suspect that both of these are caused by hasError in QueryObservable returning false if policy === 'none': https://github.com/apollographql/apollo-client/blob/905001e40cfadf841aa5a78aa428ab09f8cdf108/packages/apollo-client/src/core/ObservableQuery.ts#L56

I hope to file the PR myself, but I want to understand this design decision before I do. The distinction between result.error and result.errors is unclear at best and in this context, the role of errorPolicy="all" is confusing

tobobo commented 5 years ago

@hwillson I also have a reproduction for this case: https://codesandbox.io/s/qqqzp95vq9 , which I originally posted in this thread https://github.com/apollographql/apollo-client/pull/4237

joepuzzo commented 5 years ago

@benjamn Any updates / Comments on this issue. Does the reproduction i created accurately show the problem?

joepuzzo commented 5 years ago

@hwillson is the reproduction enough to highlight this issue? Is there anything else I can do?

joepuzzo commented 5 years ago

If I could at least get a confirmation that this is in-fact a bug and not something I'm doing wrong that would be greatly appreciated...

joepuzzo commented 5 years ago

@benjamn @hwillson I really don't mean to put pressure but I would really appreciate an update on this OR at the very least a confirmation that its a real issue.

joepuzzo commented 5 years ago

This was marked as hasProduction two months ago and nothing has been said? I want to assume i'm doing something wrong seeing as this seems like a large overlooked issue but I don't think I am.

dumaron commented 5 years ago

I think that this is a regression, and not of apollo-client but in react-apollo. I've opened this issue some times ago to talk about this bug, but it was closed since this pull request was merged in master, fixing the problem (at least, I wasn't able to reproduce it anymore).

Now I'm experiencing the same problem again. Also, other issues are being opened in those days to report similar behaviours. I can't say the version since the bugfix stopped being effective; I'm doing myself some debug, but sadly the time I can give to this process is very limited.

My proposal is to close this issue, close the other ones in apollo-client and start again in react-apollo, opening a new issue or re-opening mine. I'll try to do my best; if anyone has time to spend on it, I think that a good start can be check the code in the merged pull request.

sergelerner commented 5 years ago

FYI I've recreated @joepuzzo sandbox from above, with latest apollo versions, together with @apollo/react-hooks, using useQuery, the problem remains.

https://codesandbox.io/s/apolloerrorissue1withhooks-moee3 once you navigate to the "dog" path, second time, error disappears.

@hwillson , any news on caching errors?

PavelStsiapanau commented 4 years ago

any updates?

PinkyJie commented 4 years ago

I think this is a critical issue, without this cached error, there is no way to do error handling correctly. Error can only be showed once and once you navigate way and come back, the error is gone.

gtavidian commented 4 years ago

any updates on this? did someone come up with a work around?

dannycochran commented 4 years ago

I think this workaround works, at least with the default error policy. I think with some error policies onComplete would fire, so you'd reset your error when it could still exist. You could probably fix this to work with any error policy, you just would need to check what type it is.

import { OperationVariables } from 'apollo-client';
import { useQuery, QueryHookOptions } from '@apollo/react-hooks';

export const useWrapperQuery = <TData = any, TVariables = OperationVariables>(
  graphQlQuery: DocumentNode,
  options?: QueryHookOptions<TData, TVariables>
): QueryResult<TData> => {
  const [cachedError, setCachedError] = useState<ApolloError | undefined>();
  const queryResults = useQuery(graphQlQuery, {
    ...options,
    onCompleted: data => {
      options?.onCompleted?.(data);
      // here you might need to check your options.errorPolicy
      setCachedError(undefined);
    },
    onError: error => {
      options?.onError?.(error);
      setCachedError(error);
    },
  });
  return {
    ...queryResults,
    error: cachedError,
  };
};
KosGrillis commented 4 years ago

Can we expect this to be fixed by the Apollo team? Are there any workarounds for hook-based queries?

lifeiscontent commented 3 years ago

Ping @hwillson any updates here?

jpvajda commented 1 year ago

@joepuzzo 👋 I know this is a very old issue and the team is taking a look at it now to determine if it's still something that should remain open. Would you be able to install the latest version of Apollo Client and let us know if this is still an issue for you?

lifeiscontent commented 1 year ago

@jpvajda yes, still an issue, basically if you want to use apollo-client with SSR you can get apollo to prefetch the content of a page operation, store it into the cache and on hydration receive the cached contents.

however, in the scenario that a operation fails, you now lose that information because the apollo cache doesn't store error responses into its cache, so if an error happened, you have no idea because the error is dropped by apollo, and the only way to safely deal with is by running the operation, checking if it had an error and build some mechinism outside of apollo to determine if the operation had an error before checking the contents of the cache.

note: I'm using "operation" here as a general term to mean either "query" or "mutation" as a graphql client.

tobobo commented 1 year ago

We ended up working around this by mandating that all recoverable errors (e.g. anything that would show a page/state other than "oops, something went wrong") be represented in the schema and handled in the resolver.

dhritzkiv commented 1 year ago

@tobobo we investigated doing the same, if only to benefit from typed/schema'd errors, but ultimately came up short due to having to reimplement graphQL's field-level error logic from scratch, specific complexities with our use case, and the fact that errors may still be inevitable.

atdrago commented 1 year ago

I ran into this issue as well. I'm lazy-loading many components on a page, each of which uses the same query hook, so the problem occurs for me if one of the components loads after another component already received the data from the hook. For those who are having this issue after lazy-loading entire pages, this solution may not be much of an improvement over just setting the default fetch policy to cache-and-network.

I attempted to follow @dannycochran's example above, but felt it would be too complex and error-prone to create my own error cache, especially with our error policy set to "all".

I ended up solving the problem by temporarily switching our fetch policy for usages of the hook that I know will reproduce the issue. If the lazy-loaded component mounts and never receives loading: true from the hook, I switch the hook to use fetchPolicy: "cache-and-network" for that 1 request. That immediately displays any cached data, but still retriggers the network request so that it passes along the error. All subsequent requests use fetchPolicy: "cache-first".

This adds +1 query whenever another component (or set of components) mounts that uses a previously cached query, but still saves some network requests over setting our default fetch policy to cache-and-network.

import { useRef } from 'react';
import { useQuery as useQueryOriginal } from '@apollo/client';

export const useQuery: typeof useQueryOriginal = (query, options) => {
  const didEnterLoadingStateRef = useRef(false);

  // First, run the query with the default fetch policy.
  const queryResultWithCache = useQueryOriginal(query, options);

  if (queryResultWithCache.loading) {
    didEnterLoadingStateRef.current = true;
  }

  // If the query is loading now, or if it loaded before, then we should skip
  // the cache-and-network query.
  const shouldSkipCacheAndNetworkQuery = didEnterLoadingStateRef.current;

  const queryResultWithCacheAndNetwork = useQueryOriginal(query, {
    ...options,
    fetchPolicy: 'cache-and-network',
    skip: options?.skip || shouldSkipCacheAndNetworkQuery,
  });

  return shouldSkipCacheAndNetworkQuery
    ? queryResultWithCache
    : queryResultWithCacheAndNetwork;
};

I'm also using GraphQL Code Generator to generate hooks for my queries, so instead of using the hook directly, I set apolloReactHooksImportFrom in the codegen config, so it uses this useQuery hook instead of the one it would normally import from @apollo/client.

If this matches your use case, then you'll also need to add the following line to the above file so that it can access everything else in @apollo/client:

export * from '@apollo/client';

Hope this helps someone! And really hoping for a solution from the Apollo team so I can remove this workaround 😄

vitexikora commented 9 months ago

We just ran into this and simply cannot believe it has been like this for so long! We really need the errorPolicy 'all' and of course we need to have errors cached... 😢

Maximaximum commented 1 month ago

Wow, I'm really surprised to find out there's such a fundamental issue in apollo client and the project dev team doesn't even care to respond or acknowledge it.

And it's been alive for 5 years already! 😮

It basically makes reliable handling of the graphql errors with apollo-client impossible.

It's such a pity that adoption of such a great and useful idea (GraphQL) just got stuck due to the lack of proper attention to implementation details