2.0 not clearing error after refetch

[msmfsd]

Intended outcome:
data.error should be cleared after successful refetch.

Actual outcome:
after error a successful refetch does not clear the error.
(In fact I dont think query data is set to undefined on error either, it also retains previous response if there was one. Not sure if that caching and is possibly expected on network-only calls?)

How to reproduce the issue:

1. turn graphql server offline
2. as expected queries return an error { myData: undefined, loading: false, error: 'Error: Network error: Failed to fetch' }
3. turn graphql server back online and do a refetch
4. query returns data successfully but error is not cleared { myData: { uuid: abc123 }, loading: false, error: 'Error: Network error: Failed to fetch'  }

Version

• [email protected]

[msmfsd]

BTW I have options set to fetchPolicy: network-only

[jbaxleyiii]

@msmfsd would you be able to create a reproduction of this issue? I think I can reproduce it as well, but it would save on time if you had something I could take a look at?

[msmfsd]

@jbaxleyiii yep will have a look now

[msmfsd]

@jbaxleyiii error template is here: https://github.com/msmfsd/apollo-error-tmpt-pr-2513

NOTE: it was tricky to trigger an realistic error off and on without an external server..

[tphan18]

@msmfsd @jbaxleyiii I got the same issue. It's pretty easy to reproduce it.

1. Do a success query
2. Update the token/session in your store to get invalid token/session
3. Refresh the page -> got 401 error from graphql query -> should route to login
4. Login -> should route back to the route on step 1 but still got 401 error -> route back to login again

Quick fix is to refresh the page.

[tphan18]

I got the same bug for none and all in errorPolicy

[Wenzil]

This issue prevents letting users retry failed queries manually. I would assume this is a really common use case. Any ETA on a fix?

[adambom]

This is a really critical bug because it means that refetches won't work after a network status interruption.

[dotansimha]

Same here!
@jbaxleyiii do you have an idea for a workaround? :(

[adambom]

@dotansimha I forget where I found this, but I read on another github thread somebody suggesting the following workaround: Overwrite the GraphQL.prototype.dataForChild method so it includes the property, retry.

You should be able to find the section in react-apollo.browser.umd.js (in the react-apollo package) that contains these lines:

else if (error_1) {
    assign(data, (this.queryObservable.getLastResult() || {}).data);
}

You can change that to be this:

else if (error_1) {
    assign(data, (this.queryObservable.getLastResult() || {}).data, {
        retry: () => {
            this.unsubscribeFromQuery();
            this.queryObservable = null;
            this.previousData = {};
            this.updateQuery(this.props);
            if (!this.shouldSkip(this.props)) {
                this.subscribeToQuery();
            }
        }
    });
}

Then if your query results in a network error, you can at least call this.props.data.retry() and it preserves the ability to refetch and all that.

Instead of overwriting node_modules, the way I set this up was to drop a file into a folder in my project called hax and add it there. Here's a link to a gist of that file.

Now, you just import graphql from App/hax/graphql instead of react-apollo. It's a shitty workaround, but then again, there aren't many good options here.

The other thing you can do is use the RetryLink and make it so that it never stops retrying.

[dotansimha]

Thanks @adambom ! I used your workaround and it works! (If someone else needs it, I published is as [email protected] to NPM).

[adambom]

Glad I could help. Keep in mind that if you're doing something like this:

componentWillReceiveProps(nextProps) {
    if (nextProps.data && nextProps.data.retry) {
        nextProps.data.retry();
    }
}

You'll get an infinite loop of retries if you're not careful. You can either throttle the retry function or check to make sure that retry was not set on the previous props and do some kind of state manipulation to make sure you're not calling retry over and over.

[msmfsd]

@jbaxleyiii can the above be used as fix for this issue?

[rajit]

Hi @jbaxleyiii, do you have any ideas for the right solution for this? The react-apollo-temp package has solved the problem for me, FYI.

[adambom]

Ok, I've spent many many hours debugging this now with no luck whatsoever. I've checked both apollo-client and react-apollo to see if I could fix the error, but no matter what I try, I cannot make this problem go away.

I could really use some help from someone who understands this codebase better. This is a serious issue, and will affect any user who may have a spotty internet connection.

To summarize the problem and how to reproduce it: after receiving a network error, components wrapped with the graphql higher order component are not receiving updates when refetch is run, even though the queries are executed.

The way you can reproduce this is to set up one component with a query, wrapped in the graphql higher-order component. You will need a local apollo server to serve the requests. Load up your app, then after it's loaded, turn off the server, and cause the query to be refetched. You should get a network error.

Then turn the server back on and refetch again. In your server logs you should see that the query is being run. However, the component will not be updated with data from the server (i.e. render is not called - the component does not receive new props).

From what I can gather, after the refetch completes, QueryManager.broadcastQueries() is called. When it tries to broadcast the query that was refetched, it's essentially a no-op since this.queries[queryIdOfQueryThatFailed].listeners is an empty array.

I tried to address this by commenting out ObservableQuery.prototype.tearDownQuery, which gets called when the query errors out. I also commented out this line.

Indeed, that prevents the listeners from being removed, but when the listener is called (QueryManager.prototype.queryListenerForObserver) observer.next doesn't do anything because the observer is marked as closed (this line).

[adambom]

Ok for anyone following this issue, I have an improved workaround to the one I mentioned earlier. Instead of adding a retry method, we can just monkey-patch the refetch method like so:

else if (error_1) {
    const refetch = data.refetch;
    assign(data, (this.queryObservable.getLastResult() || {}).data, {
        refetch: (refetchOptions) => {
            this.unsubscribeFromQuery();
            this.queryObservable = null;
            this.previousData = {};
            this.createQuery(refetchOptions, this.props);
            if (!this.shouldSkip(this.props)) {
                this.subscribeToQuery();
            }
            return new Promise(function (r, f) {
                _this.refetcherQueue = { resolve: r, reject: f, args: refetchOptions };
            });
        }
    });
}

Then you can just continue to call refetch as usual. Obviously this is just another hack, which sucks, but I don't have anything better right now. Also, keep in mind that just because you'll be able to call refetch from your components doesn't mean that client.reFetchObservableQueries() will work (it won't). So yeah, there's that.

@dotansimha would you mind changing the react-apollo-temp package to reflect this behavior?

-- edit 1 -- I published this change under the name react-apollo-temp-adambom for anyone who wants to pull it

-- edit 2 -- this.createQuery(refetchOptions, this.props); should be this.createQuery(this.calculateOptions(this.props, refetchOptions));. I published version 2.0.2 with the change.

[alewiahmed]

Thanks @adambom you are a life saver. Replacing my react-apollo with react-apollo-temp-adambom Fixed the issue.

[adambom]

@jbaxleyiii, @stubailo, @helfer, @Poincare (or any of the other mainteners) would you be open to a PR for this fix? I don't think I've cured the problem, only treated the symptom, nor do I think I've found the optimal fix. That said, I'd like to help others who are having this problem and I'd really appreciate it if someone more knowledgeable with this codebase could chime in.

Thanks!

[craigbilner]

I have a similar very closely related issue to @adambom's which can be found in our storybook. Click "next page" then "retry", then all subsequent paging will be broken. Code can be found in the repo, the provider package and author-profile.stories.js are of most note.

Potentially this happens due to a mixture of issues. We're not doing paging correctly, not refetching correctly and/or the bugs highlighted above. I appreciate this criss-crosses react-apollo and here too, so happy to raise any issues over there.

When the error occurs, the query is "torn down" and the observers removed, however the query is still hanging around. The refetch that is performed in connect.js is using the one provided by data. This simply fetches the data and returns it which renders the new page without really updating anything query related. On subsequent paging the former query subscription has the isTornDown property set to false in setVariables but it's still essentially torn down and the resulting network response can't tell anyone about it with no more subs/observers, which just returns the existing data and leaving the page in the "refetched state".

@adambom's solution above works because it creates a whole new query with new subscriptions and observers for the subsequent paging to work. I wouldn't call it a hack though because it's doing everything that needs to be done, although perhaps some of this work should happen in the error handling portion?

Also happy to contribute to a PR but just need direction on how it's supposed to work.

• Should the error handling completely remove this query so it's can't be updated in the future?
• Should the isTornDown property be guarded on "torn down" queries instead? (this would then at least result in pages returning errors after the refetch)
• Does @adambom's solution to refetch make sense in that a whole new query does need to recover the broken state or should error handling not be so aggressive and tear nothing down, simply updating it's state to be in an "error" state which can be overwritten in the future?
• Should we be paging by changing props which is using setVariables on the same query incorrectly?

As a side note, data.refetch is mutated and reverted when using the above solution with the guard that's in place in graphql.js from react-apollo which results in the default refetch being used:

// handle race condition where refetch is called on child mount
          if (!this.querySubscription) {

This again may be the result of our use of graphql and/or something else?

[alewiahmed]

hey @adambom I'm having trouble using fetchMore as well after an error occurs on the first call. could you see the issues #2533 or #2539, my guess is, they are related to this issue, but I couldn't make your solutions work on fetchMore. It would be awesome if you check those out.

[adambom]

@alewiahmed I don't think I'll have time too look at it this week. The fix I issued specifically only deals with refetch and nothing more. If this is urgent, I would suggest you take a look at the dataForChild method of the graphql higher order component and try something similar to what I did with refetch. If you find a fix, I'm more than happy to publish it in my temp package.

[wdimiceli]

I am running into this issue as well.. strangely enough it seems like the unit test for this scenario was removed some months ago.

The issue is fundamentally caused by ES6 Observables terminating the stream when an error is sent to an observer. In this case, any kind of error in the query causes the react-apollo component to get dropped (from here to here to here), and the subsequent teardown of the query is just a further consequence. The components themselves don't contain much subscription logic beyond the mount/unmount lifecycle and thus remain unaware the queries were torn down.

It seems like the subscription logic for react components might need further consideration, or perhaps a different way to handle error conditions.

In either case, here is my own short-term workaround which should serve as a drop-in replacement for the normal graphql function:

const MyGraphQL = (...args) => Component =>
    class extends graphql(...args)(Component) {
        dataForChild() {
            const data = super.dataForChild();
            if (data.refetch && data.error) {
                data.refetch = (...args) => {
                    this.unsubscribeFromQuery();
                    const { lastError, lastResult } = this.queryObservable;
                    // If lastError is set, the observable will immediately
                    // send it, causing the stream to terminate on initialization.
                    // We clear everything here and restore it afterward to
                    // make sure the new subscription sticks.
                    this.queryObservable.resetLastResults();
                    this.subscribeToQuery();
                    Object.assign(this.queryObservable, { lastError, lastResult, isTornDown: false });
                    return this.queryObservable.refetch(...args);
                };
            }
            return data;
        }
    };

[msmfsd]

Great work @wdimiceli - can you explain more specifically how you propped it in to your codebase? Did you create a lib or a hoc? I'm keen to test it on a live application.

[wdimiceli]

@msmfsd In my case it was pretty easy because my components are already using a wrapped version of graphql() which adds a few extra things (specifically, some build-step oddities). I keep it in a utilities script that is imported instead of the regular HOC, so my components automatically get any improvements I make.

I'm sending a PR which incorporates the approach I took plus a unit test that demonstrates the issue. Would love to get some feedback and/or get it merged asap (...or if I did it entirely wrong because this is my first github PR...).

[msmfsd]

Thanks @wdimiceli I monitor this issue closely so will review for sure.

[roycclu]

@adambom
tried your react-apollo-temp-adambom
it worked for refetch errors.

There's another similar error, where a second user can see the error returned from the first user's query. Not refetch, but very similar concept.
Checked the apollo-client store was cleaned out after logout. So guessing the problem is also in react-apollo hoc.

[ilijaNL]

@wdimiceli Can confirm his solution works

[Dremora]

Now that this has been fixed in #1531, any idea on when a new release will be published to npm?

[fc]

@Dremora apollographql/react-apollo#1531 <-- the PR is for react-apollo, not apollo-client, better to ask there.

[hwillson]

This was fixed in apollographql/react-apollo#1531. Closing - thanks!

[harshitpthk]

was it part of 2.1.4

[almassapargali]

Still having this issue on "apollo-client": "^2.6.8"