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.35k stars 2.66k forks source link

There was a undescribed "Breaking change" in useMutation hook #9065

Open TriPSs opened 2 years ago

TriPSs commented 2 years ago

Since this commit there was a "breaking change" in the useMutation hook.

Intended outcome: When doing the following code:

const { errors } = await doMutation()

errors was filled if there was an error from the API

Actual outcome: Error is now thrown instead of returned.

How to reproduce the issue:

// Make sure this mutation returns a error
const { errors } = await doMutation()

// Errors wont be called since 
console.log(errors)

Versions Everything after 3.4.0

TriPSs commented 2 years ago

Also noticed that the following code does not work:

await doMutation({
  onError: (error) => console.log(error), // This is never called, errors keeps getting throwed
})
brainkim commented 2 years ago

The commit you’re referencing was only merged in 3.5, and was mainly a rote refactoring of the hook so I didn’t have to reason about code across 15 files, so I don’t think that’s the behavior change you’re looking for.

Might be related to this issue: https://github.com/apollographql/apollo-client/issues/8793

brainkim commented 2 years ago

Closing as a duplicate of #8793, let me know if you don’t think was right!

TriPSs commented 2 years ago

@brainkim don't really think its a related to #8793 as that ticket wants to use the onError callbacks of the doMutation and I used const { errors } = doMutation and since the error is thrown inside unless you provide a onError it will never be returned.

I think either the interface needs to be updated or (my preference) the errors should always be returned.

brainkim commented 2 years ago

@TriPSs So at one point, the execute() function didn’t reject? It just returned the errors in an array? Sorry, I don’t think this behavior is documented or unit tested, and I only started working on Apollo Client during the 3.3 days.

TriPSs commented 2 years ago

Yea, if the line where it now throws the error would return it just like it does a couple of lines above it the behaviour would be how it was.

Currently we have it this was in a lot of places, so if this behaviour won't work anymore we would have to refractor all off those and add empty try catches to it otherwise our error reporting tool will send un needed messages.

TriPSs commented 2 years ago

I think I can enable the behaviour I want by setting the following on my Apollo client:

defaultOptions: {
    mutate: {
      errorPolicy: 'all'
    }
  }

@brainkim could you confirm this?

jembach commented 2 years ago

Hi @TriPSs,

I'm having the same problem. It is in my opinion an unexpected behavior because when i can get an error returned i expect that it does not throw an error. In addition it isn't documented:

Currently my solution looks like this:


class ApolloCatchError implements ApolloQueryResult<undefined> {
  data = undefined;

  loading = false;

  networkStatus = NetworkStatus.error;

  errors = [];

  constructor(public error: ApolloError) {}
}

  async function fetchUser() {
    return gqlClient
      .query<QueryCurrentUserData, QueryCurrentUserVariables>({
        query: QUERY_CURRENT_USER,
        fetchPolicy: 'network-only',
      })
      .catch((error: ApolloError) => new ApolloCatchError(error));
  }

This is a typesafe solution providing the same result as @TriPSs and me expect

buzzb0x commented 1 year ago

Hi there, it's been months, and I see that this issue has been fixed on useLazyQuery but is still present on useMutation. Are you planning to fix it? In the meantime, I suggest updating the documentation to reflect this known issue.

Thanks for your work 🙆‍♂️

lhguerra commented 1 year ago

TriPSs comment about changing the error policy worked for me, this should be documented! The error policy default value does not work as described in the docs!

lhguerra commented 1 year ago

image

The highlighted part does not work at all, at least not in React. The onError and onComplete handlers are not executed at all when GraphQL Errors or Network Errors happen. It only worked with the "all" value, but the description for the "none" value suggests I would have at least the onError callback fired.