Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Mutation error render prop #2374

Merged
merged 5 commits into from
Sep 28, 2018

Conversation

amacleay
Copy link
Contributor

Alternative fix for #2253

FWIW, as a consumer #2253 seems like a very substantial bug. errorPolicy: all is the recommended configuration and proper error handling is the most critical path to test, but there's no good way to test graphql errors in a Mutation now except to force it to generate a runtime error using errorPolicy: none and wrap all of your mutations in a try/catch. Then again, I may just be salty about this because it took me days to understand the root of the issue.

Differences from existing PR #2297:

  • Adds tests to verify
  • Call onComplete when errors are handled graphQL errors, and do not call onError
    (Why not call onError when the error is a graphQL error? It seems like the intended use of onError is when you want to hook into the flow of unhandled errors; say, to send to an error aggregation facility, but the error really is an unexpected error and you will still ultimately end up calling throw new Error. When we get errors in the mutation's FetchResult, these are expected errors that should be handled, as opposed to code errors, and they should be handled in the render prop function)

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • If this was a change that affects the external API used in GitHunt-React, update GitHunt-React and post a link to the PR in the discussion.

This adds two tests:

* Ensure that, with the default errorPolicy, graphql errors cause the
  mutation to reject and the render prop receives an error
* Ensure that, with `errorPolicy: all`, graphql errors cause the
  mutation to resolve and the render prop receives an error

The last test case currently fails
Fixes the broken unit test introduced by previous commit
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for working on this @amacleay! Let's go with your approach of not calling onError for now. This PR addresses a pretty large shortcoming, so it will be great to get this merged sooner than later. We can re-visit the onError approach if/as needed.

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

Successfully merging this pull request may close these issues.

2 participants