apollographql / react-apollo

:recycle: React integration for Apollo Client
https://www.apollographql.com/docs/react/
MIT License
6.85k stars 787 forks source link

Global error is thrown preventing testing error states #2614

Open ryall opened 5 years ago

ryall commented 5 years ago

Intended outcome:

Error to be handled in test:

  it('shows an error and the current form when submitting with invalid data', async () => {
    const input = { email: 'user@example.com' };

    const mocks: MockedResponse[] = [
      {
        request: {
          query: RegistrationForm.mutation,
          variables: { input },
        },
        error: new Error(),
      },
    ];

    const wrapper = mount(
      <MockedProvider mocks={mocks} addTypename={false}>
        <RegistrationForm initialValues={{ email: input.email }} />
      </MockedProvider>,
    );

    const form = wrapper.find('form');
    form.simulate('submit');

    await waitForExpect(() => {
      wrapper.update();

      expect(wrapper.find('.error')).toHaveLength(1);
      expect(wrapper.find('form')).toHaveLength(1);
      expect(wrapper.find('input[name="email"]').prop('value')).toBe(input.email);
    });
  });

Actual outcome:

Error is thrown and therefore bypasses/fails test:

Error: Network error: 

      at new ApolloError (node_modules/src/errors/ApolloError.ts:56:5)
      at Object.error (node_modules/src/core/QueryManager.ts:289:13)
      at notifySubscription (node_modules/zen-observable/lib/Observable.js:134:18)
      at onNotify (node_modules/zen-observable/lib/Observable.js:165:3)
      at SubscriptionObserver.error (node_modules/zen-observable/lib/Observable.js:224:7)
      at node_modules/react-apollo/test-utils.js:963:34
      at Timeout.callback (node_modules/jsdom/lib/jsdom/browser/Window.js:678:19)

Version

GiancarlosIO commented 5 years ago

Same here 😞

no-itsbackpack commented 5 years ago

Any updates on this issue, I just run into this bug.

Update

After digging around I got this to work

class ExampleComponent extends Component {
  handleOnSubmit = async (e, mutate) => {
    e.preventDefault()
    try {
      await mutate()
    } catch {
      console.log('yea I caught the error')
    }
  }

  render() {
    return (
      <Mutation mutation={MUTATION} variables={variables}>
        {(mutate, {loading, error, data}) => {
          if (loading) return <div>loading...</div>
          if(error) retrun <div>error</div>
          <form onSubmit={(e) => handleOnSubmit(e, mutate)}>
            ...
            ...
          </form>
        }}
      </Mutation>
    )
  }
}

Adding a try {} catch {} to the async mutation call made the test pass. Open to ideas for a better solution πŸ–€

svlapin commented 5 years ago

mutate function throws if onError prop is not provided. So, another workaround would be to provide a dummy onError prop to Mutation:

<Mutation onError={() => {}}>
NickTomlin commented 5 years ago

I'm not sure I understand the context behind the default behavior, because in practice the components do properly re-render with error defined. This just seems to break the testing lifecycle. If anyone has another workaround, or a clearer understanding of the "why" behind the behavior i'd very much appreciate the context πŸ˜„

I've recently been exploring testing without MockedProvider for reasons like this: it adds a level of friction to testing components that i'd rather do without. This article goes into onesuch approach, which allows some default schema-based mocking to be overridden with custom resolvers. I've still run into this behavior but since I do not rely on MockedProvider's client instance, I can set an errorPolicy of all on mutation to receive an error:

const client = new ApolloClient({
  defaultOptions: {
    mutate: {
      errorPolicy: 'all'
    }
  },
});
hwillson commented 5 years ago

React Apollo has been refactored to use React Hooks behind the scenes for everything, which means a lot has changed since this issue was opened (and a lot of outstanding issues have been resolved). We'll close this issue off, but please let us know if you're still encountering this problem using React Apollo >= 3.1.0. Thanks!

amannn commented 5 years ago

@hwillson I've just tried with @apollo/react-hooks@3.1.0 and this problem still occurs.

I'm currently using the workaround with the try/catch to avoid the issue.

hwillson commented 5 years ago

Hi all - this can definitely be annoying, but it isn't really a bug per say. There are certain situations where people want to call the mutate function to run a mutation, and they don't care about rendering the results. They still want to know if there were problems running the mutation however, which is why the mutate function throws an error if one occurred. As mentioned above, if you don't want to use the mutate error you can swallow it. If you do, the error is still returned as error in the mutation result object.

If you define an onError callback, this is done automatically (as mentioned in https://github.com/apollographql/react-apollo/issues/2614#issuecomment-465160041) . So you can leverage that approach to avoid having the error thrown when calling mutate. While this works, it's more of a side effect of using onError than an explicit API feature.

I'm open to suggestions on how people think we should handle this, as long as the suggestion isn't a breaking change (assuming everyone wants this fixed quickly). We aren't releasing further React Apollo major versions (since this project is being merged into Apollo Client).

Should we add a new option that can be passed into the mutate function, that will explicitly identify you don't want the error thrown? Maybe something like { swallowErrors: true }?

Fire over your ideas - thanks!

jannnik commented 5 years ago

I have the same annoying problem and would like something like the swallowErrors option.

maapteh commented 4 years ago

For our application, we didn't expect the lazy mutation actually needed the onError because else it would really throw the error. I always thought it would give only an error object, but then the code showed it actually throws the error. We found out by adding a test case for it :(.

So @hwillson I think it should be more clear from a documentation perspective that a mutation will throw an error...

chrisfosterelli commented 4 years ago

@hwillson I agree this is tricky and I hate to provide more issues instead of solutions but I think it really raises a pain point, even moreso with the react-hooks than HOC components.

If we have a basic form implemented using the current instructions in the docs for performing mutations:

function SignupForm() {
  const [mutate, { loading, error, data }] = useMutation(...)

  function onSubmit(data) {
    mutate({ variables: { data } })
  }

  return (
    <Container>
      <Form onSubmit={onSubmit}>
        /* Omit for brevity */
      </Form>
      {error && (
        <Alert severity="error">
          Hm, an error occurred. Try again.
        </Alert>
      )}
      {data && (
        <Alert severity="info">
          Got a response: {data}
        </Alert>
      )}
    </Container>
  )
}

Then we get some unintuitive behaviour out-of-the-box. The mutation is going to trigger, the effect internal to useMutation is going to update, and SignupForm will re-render with new values from the useMutation hook.

However, also in the context of the previous function invocation's closure mutate will throw an exception as an uncaught promise. IIRC even if the function is ignoring the error field on the useMutation hook and prefers to catch an exception from mutate(), it's going to be running that response code in an old closure. Which I suppose isn't the end of the world and we have to embrace to some degree with hooks.

But it's also confusing for tests, and seeing uncaught exception errors in the browser. It's particularly nasty in tests because I'm not sure how to get the tests to pass -- I'm testing what the user sees and looking for an alert error dialogue to show up in the DOM. It does this, and then after the test teardown an exception is thrown because of the uncaught promise which Jest really doesn't like. The uncaught exceptions in the browser aren't the end of the world either but are also just visual noise in the console for an error that is actually being "handled".

Of course I can just wrap it in an empty try-catch but that feels really hacky as the default way to use the new API.

I think this also relates to https://github.com/apollographql/apollo-client/issues/3876 and maybe #2737