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

Include cached data with error #548

Merged
merged 18 commits into from Apr 21, 2017

Conversation

Projects
None yet
3 participants
@StevePotter
Copy link
Contributor

StevePotter commented Mar 20, 2017

This PR includes data cached from the Apollo store when there is an error while fetching the query. Now useful data can be displayed along with any error message.

For example, the server is down. An error will be returned as it always was. But until now, the data prop was empty. A good UI for this situation would be to display an error message like "There was an error reaching our servers. Until the problem is fixed, you can still use the app. Your data might just be a bit off." and show the cached data. Now you can!

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Mar 21, 2017

Can you add a test for this as well?

@helfer
Copy link
Member

helfer left a comment

Thanks for the PR @StevePotter. I think this is a behavior we should change in Apollo Client, but sort of that, this might be a good PR to patch things up. A test is definitely needed though.

@@ -1,6 +1,6 @@
{
"name": "react-apollo",
"version": "1.0.0-rc.3",
"version": "1.0.0-rc.4",

This comment has been minimized.

@helfer

helfer Mar 21, 2017

Member

We'll update the package.json ourselves, this should not be in here.

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

### vNext

### 1.0.0-rc.4

This comment has been minimized.

@helfer

helfer Mar 21, 2017

Member

You can just put this under vNext, and we'll figure out which version we'll release it under.

if (error) {
let previousResult = this.queryObservable.getLastResult();
if (previousResult && !previousResult.error) {
dataToMerge = assign({}, previousResult.data, currentResult.data);

This comment has been minimized.

@helfer

helfer Mar 21, 2017

Member

I think it's pretty hard to predict what data you'd get here if there ever is any data in currentResult. I think it might be better to just return the previous result.

This comment has been minimized.

@StevePotter

StevePotter Mar 22, 2017

Author Contributor

Hey @helfer thanks for the review. I'm happy you're encouraged by this PR. Please give me a few days to update the PR as I'm swamped on a few higher priority issues. I'll get the appropriate testes in there and clean up the currentResult stuff and just use getLastResult().

Thanks again!

@StevePotter

This comment has been minimized.

Copy link
Contributor Author

StevePotter commented Apr 10, 2017

@helfer I tightened up the code and added test coverage. This one is good to go.

The test:

  1. Run a successful query
  2. Refetch, receiving an error
  3. Verify the data from 1 is included alongside the error.

The test fails without my change to src/graphql.tsx and passes with the change.

@StevePotter

This comment has been minimized.

Copy link
Contributor Author

StevePotter commented Apr 13, 2017

@helfer how about now? It's clean, tested, and green.

@StevePotter

This comment has been minimized.

Copy link
Contributor Author

StevePotter commented Apr 20, 2017

Another PR I submitted to apollo-client that doesn't affect this one but it needed for a solid offline UX apollographql/apollo-client#1601

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Apr 21, 2017

Thanks @StevePotter, the test looks good!

This could technically break apps if a component only checks for data but not error, but I'm inclined to include it in the next minor release because we've been printing a warning to the console for quite some time now for unhandled errors, and I think this is a feature that a lot of people want.

@helfer

helfer approved these changes Apr 21, 2017

@helfer helfer merged commit 0cd8c12 into apollographql:master Apr 21, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 92.411%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment