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

Resuscribe after error in Query component #1580

Merged
merged 1 commit into from Jan 25, 2018

Conversation

Projects
None yet
3 participants
@leoasis
Copy link
Collaborator

leoasis commented Jan 25, 2018

This PR ports the fix done in #1531 to the graphql HoC into the new Query component. Also does the same for #378.

Pinging @wdimiceli since I wrote the same method in Query slightly different and the test passed, so double checking with him. Particularly interested about the order of unsubscribing and subscribing to the new one, since I changed that to be able to reuse a method, and it seems it doesn't change the behavior.

Btw that looks like a "hacky" way to fix it, we should definitely find a way to fix this in apollo client (especially due to the lack of loading state in the fetch of the result after the error. See the test for more details).

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Jan 25, 2018

@leoasis I agree we should fix this on apollo-client. I'll make a note to take a look into observable recovery automatically in core.

Otherwise this should work just looking at it and the test. I'm good to merge this in!

@jbaxleyiii jbaxleyiii merged commit 91c1b3f into apollographql:master Jan 25, 2018

3 of 4 checks passed

bundlesize ./lib/react-apollo.browser.umd.js: 5.95KB > maxSize 5.9KB gzip
Details
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 96.078%
Details

@leoasis leoasis deleted the leoasis:resuscribe_on_error_query_component branch Jan 25, 2018

@wdimiceli

This comment has been minimized.

Copy link
Contributor

wdimiceli commented Jan 25, 2018

@leoasis I would also agree it's somewhat of a workaround rather than a full-fledged solution.

So I structured my code carefully to avoid the teardown logic. Basically, if a component is the last observer when it unsubscribes, the observable goes through the whole teardown process, and resubscribing then triggers the startup logic which has further side effects. My fix clears the handle and then resubscribes so we briefly have two subscribers before removing the original, sidestepping that whole process.

I'm sure the direct approach works too, but I was hoping to avoid unnecessary re-renders or other unexpected things that happen as a result of an observable restarting. Perhaps I was overthinking it.

@leoasis

This comment has been minimized.

Copy link
Collaborator Author

leoasis commented Jan 26, 2018

Hmm looking quickly at the code in apollo-client I see what you say, there are some extra things being done when setting up/tearing down the observable. At the same time though, the test didn't change in the amount of renders in the component, which is why I'll see if I can understand a bit more how that impacts. But thanks for the insight, that's very helpful.

@leoasis

This comment has been minimized.

Copy link
Collaborator Author

leoasis commented Jan 26, 2018

Ok looked a bit at the code, and it's a bit complicated, but I think I understand how it works.

It seems that zen-observable closes and cleans up the subscription whenever there's an error. This is written in its documentation: https://github.com/zenparsing/zen-observable#new-observable--subscribe- (check the error bullet). So technically there's no reason to unsubscribe in resubscribeToQuery, since when the error event finishes, zen observable will proceed to unsubscribe it and run the cleanup function (which is out custom function defined in ObservableQuery's onSubscribe).

Trying to unsubscribe from within the subscription error handler doesn't do anything, since the subscription by that time is already closed (not yet cleaned up), so any calls to unsubscribe are basically noops. After the error handler finished, the subscription will be cleaned up (which will not call tear down on the query because we added a new subscription by that time, so the observers are still not empty).

I think this is the underlying issue in apollo client, since by definition zen observable terminates the subscription on error, apollo client somehow needs to be able to avoid this. This also makes me wonder if that behavior is part of the standard specification for observables (haven't yet looked at it), it seems it is https://github.com/tc39/proposal-observable/blob/master/src/Observable.js#L197.

cc @jbaxleyiii which may be interested in this (unless are already aware of this 😛 )

@leoasis

This comment has been minimized.

Copy link
Collaborator Author

leoasis commented Jan 26, 2018

Here are some good ideas to avoid this behavior when we need fault tolerance tc39/proposal-observable#177, some options that I see: wrapping the observable with another, or maybe better to just not error when an error happens, but next it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment