[useMutation] unexpected handling of mutations vs queries on error handling

[Michael-M-Judd]

I'm curious about this useMutation error handling here:

apollo-client/src/react/hooks/useMutation.ts

Line 211 in c95848e

// TODO(brian): why are we returning this here???

I was really not expecting this behaviour because its different between useMutation and useQuery, where on a failure, we end up throwing a promise only if there is no onError handling.

The impact of this was that in my jest test, I was testing a component that uses a mutation and correctly handling error via the hook error return, but I was receiving an unhandled promise rejection which ended up failing my jest test. A very simple reproduction can be shown here:

import React, { useEffect } from 'react';
import { useMutation, useQuery } from '@apollo/client';
import gql from 'graphql-tag';
import { render, screen } from '@testing-library/react';
import { getMockProviderStack } from '@/jstools-test-web';  // just an apollo provider here 
import { mockServer } from '@/jest-msw'; // just mockServiceWorker
import { graphql } from 'msw';

const basicMutation = gql`
  mutation BasicMutation {
    writeSomething
  }
`;

const ComponentWithMutation = () => {
  const [mutate, { error }] = useMutation(basicMutation);

  useEffect(() => {
    mutate({
      // If this is not included, the test will fail due to an unhandled promise rejection!! 
      // onError: (e) => {
      //   console.warn('has an error', e);
      // },
    });
  }, []);

  if (error) return <p>Something went wrong</p>;

  return <p>things are good</p>;
};

describe('mutation - when failure', () => {
  beforeEach(() => {
    mockServer.use(
      graphql.mutation('BasicMutation', (req, res, ctx) => {
        return res(ctx.errors([{ message: 'Bad stuff' }]));
      }),
    );
  });

  it('should pass pass but ', async () => {
    render(<ComponentWithMutation />, {
      wrapper: getMockProviderStack().wrapper,
    });

    expect(await screen.findByText('Something went wrong')).toBeTruthy(); // this succeeds, but the test fails due to an unhandled promise rejection!
  });
});

Is there a reason we want to handle this different than that of a useQuery or useLazyQuery call?

[phryneas]

It seems like this behaviour was introduced in 3.3.15 via #8018.

At this point, I have to be honest: this has been around for so long that I think we can't change it easily, even if it is inconsistent.

I'll bring it up with the team, but my gut feeling is that this would need to be addressed in a major release - and even then a lot of people would probably still miss it and get bitten by it 😕

[phryneas]

I guess we actually did discuss that - I checked old meeting notes: this is on our TODO list for a 4.0 release.

So.. I can't offer you an immediate fix, but can tell you that we're aware and planning to change this.

[github-actions]

We're closing this issue now but feel free to ping the maintainers or open a new issue if you still need support. Thank you!

[jerelmiller]

Keeping this open to track for 4.0