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

[Breaking change] Unify error handling #352

Merged
merged 11 commits into from
Jul 11, 2016
Merged

[Breaking change] Unify error handling #352

merged 11 commits into from
Jul 11, 2016

Conversation

Poincare
Copy link
Contributor

@Poincare Poincare commented Jul 1, 2016

Unifying the error handling so that both GraphQL errors and network errors will be presented in the form of rejected promises or errors on observers. This makes it easier to deal with both types of errors and not have to always introduce two different types of error handling code.

TODO:

  • Update CHANGELOG.md with your change
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

@zol zol added the in progress label Jul 1, 2016
@Poincare
Copy link
Contributor Author

Poincare commented Jul 5, 2016

@stubailo should be ready for review

@@ -4,6 +4,7 @@ Expect active development and potentially significant breaking changes in the `0

### vNEXT
- Added support for query composition through fragments [Issue #338](https://github.com/apollostack/apollo-client/issues/338) and [PR #343](https://github.com/apollostack/apollo-client/pull/343)
- Unified error handling for GraphQL errors and network errors. Both now result in rejected promises and passed as errors on observables through a new `ApolloError` type. This is a significant departure from the previous method of error handling which passed GraphQL errors in resolvers and `next` methods on subscriptions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a breaking change, can we mention this in the changelog? Along with issue/PR numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, although we don't seem to have an associated issue so I just linked this PR which describes what the issue is as well.

@Poincare Poincare changed the title [WIP] Unify errors Unify error handling Jul 5, 2016
@stubailo
Copy link
Contributor

stubailo commented Jul 6, 2016

This looks great to me. Given that this is a big breaking change, I think we should prepare some other stuff to merge at the same time. Can you submit a PR to the docs for this as well?

@jbaxleyiii we probably have to do some stuff in react-apollo to support this, right? Perhaps we can just now pass through one ApolloError object, and people can read the properties off there?

@kamilkisiela do you think we need to do anything new to support this in angular2-apollo?

@kamilkisiela
Copy link
Contributor

@stubailo No change needed 👍

So it won't be a breaking change in angular2-apollo or anything like that.

@Poincare
Copy link
Contributor Author

Poincare commented Jul 6, 2016

@stubailo Sure, I'll write up some docs that describe how ApolloError works and submit a PR for that.

@stubailo stubailo changed the title Unify error handling [Breaking change] Unify error handling Jul 6, 2016
@Poincare
Copy link
Contributor Author

Poincare commented Jul 6, 2016

@stubailo Added the above PR to apollostack/docs. I guess we can merge this + docs once we've introduced other breaking changes as well.

@kamilkisiela
Copy link
Contributor

@Poincare thanks for updating docs for angular2! :)

@stubailo stubailo added this to the 0.4.0 breaking changes milestone Jul 11, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
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.

4 participants