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

Errors lost on cached results #4644

Open
joepuzzo opened this issue Mar 29, 2019 · 26 comments
Open

Errors lost on cached results #4644

joepuzzo opened this issue Mar 29, 2019 · 26 comments

Comments

@joepuzzo
Copy link

joepuzzo commented Mar 29, 2019

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
Copy link
Member

hwillson commented Apr 5, 2019

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

@joepuzzo
Copy link
Author

joepuzzo commented Apr 5, 2019

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

@joepuzzo
Copy link
Author

joepuzzo commented Apr 5, 2019

@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
Copy link
Author

joepuzzo commented Apr 5, 2019

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
Copy link

t3dotgg commented Apr 11, 2019

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':

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
Copy link

tobobo commented Apr 11, 2019

@hwillson I also have a reproduction for this case: https://codesandbox.io/s/qqqzp95vq9 , which I originally posted in this thread #4237

@joepuzzo
Copy link
Author

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

@joepuzzo
Copy link
Author

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

@joepuzzo
Copy link
Author

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
Copy link
Author

joepuzzo commented May 8, 2019

@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
Copy link
Author

joepuzzo commented May 30, 2019

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
Copy link
Contributor

dumaron commented May 30, 2019

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
Copy link

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?

@hwillson hwillson self-assigned this Aug 17, 2019
@PavelStsiapanau
Copy link

any updates?

@PinkyJie
Copy link

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
Copy link

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

@dannycochran
Copy link
Contributor

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,
  };
};

benjie added a commit to graphile/starter that referenced this issue Mar 23, 2020
benjie added a commit to graphile/starter that referenced this issue Mar 23, 2020
@KosGrillis
Copy link

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

@lifeiscontent
Copy link
Contributor

Ping @hwillson any updates here?

@jpvajda
Copy link
Contributor

jpvajda commented Dec 13, 2022

@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?

@jpvajda jpvajda added the 🏓 awaiting-contributor-response requires input from a contributor label Dec 13, 2022
@lifeiscontent
Copy link
Contributor

lifeiscontent commented Dec 19, 2022

@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
Copy link

tobobo commented Dec 19, 2022

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
Copy link

@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.

@bignimbus bignimbus removed the 🏓 awaiting-contributor-response requires input from a contributor label Jan 18, 2023
@atdrago
Copy link

atdrago commented May 17, 2023

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
Copy link

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
Copy link

Maximaximum commented Aug 7, 2024

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

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

No branches or pull requests