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

Query with errorPolicy "all" cannot use onError, onCompleted effectively #6966

Open
tdeedlesATX opened this issue Sep 3, 2020 · 5 comments

Comments

@tdeedlesATX
Copy link

tdeedlesATX commented Sep 3, 2020

Hi 👋 ! A team of us are working on transitioning a production application to Apollo with use of hooks in React. We have the following use case:

  • use a query with errorPolicy set to "all" (so that error and data may both have value)
  • use onError, onCompleted attributes to perform some side effects

Intended outcome:

I'd expect to be able to perform "completed" and "errored" functionality if the errorPolicy="all", since this would be a congruent outcome of the data, error, loading pattern commonly used when both data and error may be populated with this error policy.

Actual outcome:

If there is a partial error from one of the fields, only onError called (since I've defined both).
I think issue can be found in this either/or method (here in react-apollo, or here in apollographql/apollo-client).

I wonder: could onError receive data as a 2nd ordinal argument in the event that there is both an error and data?

How to reproduce the issue:

I've reproduced here: https://codesandbox.io/s/inspiring-surf-pcrgj?file=/src/App.js

Here's an inline example:

import {useQuery} from '@apollo/react-hooks`
import gql from 'graphql-tag'

const someSideEffect = (error, data) => {
if (!data) console.error(error)
 else if (data && error) {
  console.info(error)
  alert(`some of the data is shiny: ${data.isShiny}`)
 } else {
   console.log('no error 🙌')
 }
}

const QUERY = gql `
query get_stuff {
 stuff {
  isShiny
  someFinnickyFieldThatErrorsOutSometimes
 }
}
`

function Stuff() {
 const {data, loading} = useQuery(QUERY, {
  errorPolicy: 'all',
  onCompleted: (data) => someSideEffect(null, data),
  onError: (error) => someSideEffect(error),
 });

 if (loading) return 'loading...'
 if (!loading && data) return <div>{JSON.stringify(data, null, 4)}</div>;
}

Solution Proposal

Possible solution 1: provide data as a 2nd ordinal argument to onError

const {data, loading} = useQuery(QUERY, {
 errorPolicy: 'all',
 onCompleted: (data) => someSideEffect(null, data),
 // Perhaps It'd be SO nice if onError had access to data ❤, but it does not 💔
 onError: (error, data) => someSideEffect(error, data),
});

One might also argue additionally (or alternatively) that error in the onCompleted callback should be made available. Hence, in the case of errorPolicy="all" this dichotomy between "completed" and "errored" is somewhat difficult to discern. Hence:

Possible Solution 2: Perhaps if developer does not define onError and errorPolicy is "all", then onCompleted is always called with both data and error?

Possible Solution 3: Alternatively, and more generally it should just always call "onCompleted" with access to both error and data when both are defined (errorPolicy="all"). And we remove the notion of onError entirely?**

In summary, there are many solutions to this perceived bug, and the API seems unclear for this use case. These three solution proposals above are ranked in order from least to most invasive.

Version

@apollo/react-components@3.1.5

System:
OS: macOS Mojave 10.14.6
Binaries:
Node: 12.16.1 - ~/.volta/tools/image/node/12.16.1/6.13.4/bin/node
npm: 6.13.4 - ~/.volta/tools/image/node/12.16.1/6.13.4/bin/npm
Browsers:
Chrome: 83.0.4103.116
Edge: 83.0.478.61
Firefox: 76.0
Safari: 13.0.4

(FYI originally reported here, moving)
apollographql/react-apollo#403

@llamadeus
Copy link

I'm currently having some troubles when setting errorPolicy: 'all' because the onCompleted callback is being called even if data is null. This will result in null being passed for the data argument but the function signature does not reflect this circumstance.

I'm using "@apollo/client": "^3.2.7".

@jwm0
Copy link

jwm0 commented Dec 7, 2020

It's not perfect but you can use client.readQuery inside of onError to read the data that was fulfilled.

@tdeedlesATX
Copy link
Author

tdeedlesATX commented Dec 7, 2020

Thanks @jwm0 ! Your suggestion is an adequate workaround. I do not love though that this workaround takes an imperative approach to a defect in the declarative onError.

But your solution is better than what we currently have as workaround! Thank you ❤️

@zorji
Copy link

zorji commented Jul 30, 2021

I am testing with apollo/client@3.4.1, I got a completely different result.
When errorPolicy=all, it does not trigger onError on partial failure.

I can't find the official explanation on the website, when errorPolicy=all and when there is a partial error, should it trigger onError?

I found this PR #6492 and it looks like it was not expected on trigger onError.

Relevant issue for me in vue-apollo https://github.com/vuejs/vue-apollo/pull/1225/files#r679619257

@ikushlianski
Copy link

I faced the same issue with errorPolicy: 'all', I only received the partial result inside onCompleted callback of useQuery, but no errors.

I created an ErrorLink like this:

private createErrorLink = () => {
    return onError(
      ({ operation, response, forward, graphQLErrors, networkError }) => {
        return forward(operation).map((response) => {
          if (response?.data) {
            response.data.errors = graphQLErrors;
          }

          return response;
        });
      }
    );
  };

Now inside my onCompleted callback I get my data as well as errors. You will have to tweak Apollo types a bit, because it does not have errors field by default.

const [getAnalyses] = useLazyQuery<
    GetAnalysesQuery & { errors: GraphQLError[] }
  >(GetAnalysesDocument, {
    variables: { environmentContexts },
    client: quicksightApolloClient,
    fetchPolicy: "no-cache",
    onCompleted: ({ getAnalyses, errors }) => {
      console.log("errors from BE", errors);
      // other logic
    },

    onError: (error) => {
      // we get here only in case of NetworkError
  });

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

6 participants