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

Throwing an Error doesn't cause an Error to be thrown #31

Closed
lorensr opened this issue Nov 16, 2020 · 7 comments · Fixed by #33
Closed

Throwing an Error doesn't cause an Error to be thrown #31

lorensr opened this issue Nov 16, 2020 · 7 comments · Fixed by #33
Labels

Comments

@lorensr
Copy link
Contributor

lorensr commented Nov 16, 2020

I have the following in my app: doMutation().catch(e => doSomething(e.graphQLErrors)).

  • When I use my app and there's an error in the mutation, doSomething() is called.
  • When I return an error from my ApolloMockedProvider custom resolver, doSomething() is not called.

Repro: https://github.com/lorensr/apollo-mocked-provider/tree/repro-no-throw

The below console.log is not called when running npm test -- -t "mock Mutation"

lorensr@1682ac4

Note that if you add a .then(response), the error is contained in response.errors.

@lorensr
Copy link
Contributor Author

lorensr commented Nov 16, 2020

This issue is fixed by removing this line:

https://github.com/benawad/apollo-mocked-provider/blob/master/src/createApolloMockedProvider.tsx#L52

However, this line was added for a reason:

#4 (comment)

We could replace it with the defaultOptions parameter @NickTomlin suggested. This would be a breaking change, but seems better to me. Alternative would be to keep the 'all' default and add defaultOptions so people like me can override it back to the AC default of none.

@benawad
Copy link
Owner

benawad commented Nov 16, 2020

I'm good if you want to add the breaking change

@sarfata
Copy link
Contributor

sarfata commented Nov 17, 2020

@lorensr what does your server return when there is an error? What type of errors trigger the catch()?

I am having a hard time finding a definitive reference on this but in my experience a lot of errors do not trigger the catch and just return an ApolloError in the mutation result. I think that is what the current code simulates and for me it seemed to match behavior in "real life" (but this is a confusing area for sure).

From the doc:

Mutation Result

  • error | ApolloError | Any errors returned from the mutation

@lorensr
Copy link
Contributor Author

lorensr commented Nov 17, 2020

@sarfata I imagine many people either set the errorPolicy and/or use an error link to catch errors, in which case a mutation promise wouldn't reject. But if the errorPolicy is none, GraphQL errors (errors field in the response JSON) are treated like network errors, which in my experience are rejected. It looks like useMutation doesn't say when mutate promise rejects, but the AC function says it does:

This resolves a single mutation according to the options specified and returns a Promise which is either resolved with the resulting data or rejected with an error.

https://www.apollographql.com/docs/react/api/core/ApolloClient/#ApolloClient.mutate

The code I use that results in a GraphQL error is throw new Error('unauthorized') from a resolver in Apollo Server.

{"data":{"removeReview":null},"errors":[{"locations":[{"column":35,"line":1}],"message":"unauthorized","path":["removeReview"]}]}

@lorensr
Copy link
Contributor Author

lorensr commented Nov 17, 2020

Working on the test for the PR and ran into this:

testing-library/react-testing-library#828

and I got dejavu 😆

@sarfata
Copy link
Contributor

sarfata commented Nov 18, 2020

Yeah sounds like this is a poorly documented behavior (or too flexible with errorPolicy). I still dont understand exactly how it is supposed to work but dont hold up for me! thanks for taking the time to research.

@github-actions
Copy link

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants