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

Resubscribe after error -- apollo-client issue 2513 #1531

Merged
merged 6 commits into from Jan 17, 2018

Conversation

Projects
None yet
10 participants
@wdimiceli
Copy link
Contributor

commented Jan 9, 2018

This is a fairly minimal fix for React components ceasing to get updates after an error occurs in a query. It happens due to the way observables work.. they will terminate a subscription after the error handler is called. Without going through and reconsidering all of the subscription logic between react-apollo and the client, simply resubscribing on error seems like the simplest way to get it working again. A unit test was written to cover this scenario.

Here is the most relevant issue:
apollographql/apollo-client#2513
I have seen several others which might be resolved with this fix.

wdimiceli added some commits Jan 8, 2018

Add resubscribe() method and call it when a query errors.
Hotfix for apollo-client issue 2513.

When a query has an error, Zen Observables will terminate the
subscription, so the react component no longer receives updates.
This commit adds a simple resubscribe method and calls it when
an error occurs.
@meteor-bot

This comment has been minimized.

Copy link

commented Jan 9, 2018

@wdimiceli: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@rosskevin

This comment has been minimized.

Copy link
Collaborator

commented Jan 10, 2018

@excitement-engineer if this looks good, we need to mirror the tests and changes to Query.

@wdimiceli if you want to increase bundle maxSize in the package.json and push it that will solve the build error. 4.8 will be fine.

@rosskevin
Copy link
Collaborator

left a comment

This looks good to me, but I'm no authority in this area. I would prefer @jbaxleyiii and @excitement-engineer give their approval.

@jbaxleyiii jbaxleyiii merged commit e3f98a8 into apollographql:master Jan 17, 2018

4 checks passed

CLA Author has signed the Meteor CLA.
Details
bundlesize ./lib/umd/react-apollo.js: 4.73KB < maxSize 4.8KB gzip (63B larger than master, careful!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 96.246%
Details
@edorivai

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

Is there a chance this will be released on NPM any time soon? Pretty, pretty please 😄

@rosskevin

This comment has been minimized.

Copy link
Collaborator

commented Jan 26, 2018

Please check the releases tab before posting. We are going to try and release more frequently and this most certainly has been released (though strangely danger did not require a changelog entry for it).

Try the 2.1 beta: https://github.com/apollographql/react-apollo/releases

@edorivai

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

@rosskevin Sorry, wasn't aware of the beta releases, just checked npm stable. Thanks!

@nfantone

This comment has been minimized.

Copy link

commented Feb 3, 2018

Unfortunately, this didn't resolve #1385 for me 😞 . However, installing the beta release of react-apollo + latest apollo-client@2.2.2 + apollo-cache-in-memory@1.1.7 made requests work again after an error and the loading is now being set correctly.

@nfantone nfantone referenced this pull request Feb 8, 2018

Closed

reset last results on new observer subscribe #2871

0 of 3 tasks complete
@AndrewHenderson

This comment has been minimized.

Copy link

commented Feb 9, 2018

@nfantone I have upgraded to all of those versions with react-apollo@2.1.0-beta.2.

I'm finding that when their is an error in a component, the loading prop is false, however none of the props are passed to the component.

I'm using a sub-resolver and testing when there is an error in that single "sub-function."

Everything works as expected in GraphQL – when there is an error, that single property is null, partial result is returned, and the GraphQL error is returned on the response.

However, because there is an error, none of the partial result gets passed to the component 😕

@edorivai

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

@AndrewHenderson I might be misunderstanding, but partial results sound like a job for errorPolicy: https://www.apollographql.com/docs/react/features/error-handling.html#policies

@AndrewHenderson

This comment has been minimized.

Copy link

commented Feb 9, 2018

@edorivai That’s exactly the setting I needed. I completely missed that. Thank you! 😁

@nfantone

This comment has been minimized.

Copy link

commented Feb 9, 2018

@AndrewHenderson Actually, what I'm seeing is that now the loading and error properties are working fine, but - on error, data continues to hold the last correctly fetched data, instead of what was actually returned by the /graphql response.

@edorivai Would you have any input on this? Would this be the intended functionality?

@AndrewHenderson

This comment has been minimized.

Copy link

commented Feb 9, 2018

@edorivai I tried using errorPolicy, but I’m still experiencing what @nfantone has described.

I thought it might work using the new Query component, but don’t see an errorPolicy propType for that new component.

@edorivai

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2018

@tmeasday

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

I could be misunderstanding but I suspect this does not work with polling queries (I haven't been able to make it work), because apollo-client does not resume polling after a network error

Perhaps if the query has pollInterval set react-apollo should also re-create the observableQuery on error also?

@404sand808s

This comment has been minimized.

Copy link

commented Jun 14, 2018

Hi guys, checking in to see if anybody knows when this might be released — desperately need this to re-establish connection after an error (to give the user a way to reload). Thanks, and sorry for the bump.

@iamrommel

This comment has been minimized.

Copy link

commented Aug 21, 2018

This is really breaking, we are are using react-native and apollo, and whenever i go to elevator (which most of the time has not data connection), my app gets error, and even when i get back to wifi connection, my connection cannot be recovered. Correct data are being pull from the server, but the component are not updated

rexmac added a commit to Shopify/react-apollo that referenced this pull request Jan 14, 2019

rexmac added a commit to Shopify/react-apollo that referenced this pull request Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.