Server-returned GraphQL error does not reset data on component

[alexzielenski]

Information

I'm running a query along the lines of:

query BoardDetail($id: ID!) {
    board(id: $id) {
        id
        status
        name
    }
}

I'm also using react router to page between a component which lists such boards and links to the detail page. For one of my board objects my status field result is incorrectly returned by the server (it is a GraphQL enum type and I'm returning an invalid value).

Steps to Reproduce

1. Start at board list
2. select a board whose status field is valid and has no error. It's page renders fine.
3. Go back to board list.
4. Select a board whose status field is invalid and returns a GraphQL error. It's page displays the error and shows the board from step 2.

Buggy Behavior

props.data.board is populated with the result of a past render while props.data.error is populated with the error for the current query. This puts my UI in an inconsistent state showing both an error and a successful render of a board viewed in the past. I know that I could do something like if (data.error != null) {...} else { <Board /> } but what if I want to display an error at the same time as receiving cache data?

I think this problem stems from some behavior Apollo has that when a query fails it reverts to a past result to use as data for the component. This is fine except for when we use variables in our queries. When I run my query in the devtools with "Load from Cache" enabled and correct id variable, I get back the (stale) data that I want.

Expected Behavior

props.data.board is null or cache-loaded (with correct variables) and props.data.error is non-null as the server returns.

Version

• [email protected]
• [email protected]

[helfer]

Hey @alexzielenski, thanks for filing an issue. You're right that having old data along with the new error is inconsistent. The reason for it is that Apollo Client currently doesn't put errors in the store, which also means that it won't return data and errors at the same time. If there's an error, it only returns the error. In the past this has led to an inconvenient situation for people where they lose all the data in their component when an error occurs, so we decided to accept a PR that provided the old data with the new error.
What I recommend doing is simply not displaying the data if there's any error, like you suggested yourself. We're working on a new store that will have more intuitive behavior around errors and pass them through to components along with the new data.

[alexzielenski]

Hi @helfer! Thanks for your rapid response.

Would it be feasible to change react-apollo in the interim so it runs your query on the cache again, rather than simply returning the last-rendered data by that component? I think this would be the correct behavior and would not change components which are relying on using previous query results. This would be acceptable to me, since then my detail page would not display the wrong document; but a stale version.

[helfer]

@alexzielenski unfortunately that's not possible because if there's any error, the new data doesn't get written to the cache (because it might overwrite perfectly good old data with an error, which could affect other queries as well). Not sure if there's a good workaround, but we're working on fixing the problem.

[alexzielenski]

@helfer I meant that upon error, nothing gets written to the cache but the query should be simply re-evaluated as cache-only and data.error is populated along with it.

[helfer]

@alexzielenski ah, I see what you mean. Would that return something different in your case? Theoretically if you're watching the query, you should already have the latest result from the cache when the error comes in, so simply returning that with the error as we do now should lead to the same result.

[alexzielenski]

@helfer Since I am using query variables the result would indeed change.

[helfer]

Makes sense. However, what would you expect to get when that query isn't in the store? We don't do partial results any more, so you'd get nothing.

[alexzielenski]

Shouldn't I get nothing rather than the result of a completely different query (the data is completely unrelated due to variable difference)? I feel like use cases call for the old data typically do not use variables in their queries.

To attack this specific use-case Apollo could search the variables for one of type GraphQLID! but that's probably too much.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

[jbaxleyiii]

@alexzielenski I think the current behavior (the RA one, AC still needs work regarding errors) is the best suited for most applications. Since there is a workaround, I'm going to close this issue. Feel free to reopen if you have further thoughts

[alexzielenski]

Hi @jbaxleyiii. I don't think the change I have proposed would alter the current behavior. It would simply prevent unrelated queries from being provided as the data for some pages. I don't think I'm being understood here since no one has replied with a reason why this isn't coherent?

All I am saying is:
In the result of an error, instead of returning the last returned query result
Run the current query on the store and return that result

Since the last returned query result was committed to the store, this will have no effect on current implementations relying on this behavior. All this changes for current implementations is that the query will have their updated variables so if I'm viewing a page for user/123which gives me an error after user/1 succeeded in a render, I'm not viewing the incorrect user and instead can deal with either stale data or null. This use case seems pretty broad to me.

[stantoncbradley]

@alexzielenski @jbaxleyiii 100% agree with alexzielenski. We just encountered this issue and the current behavior is out of left field.

We have a list of documents.
User taps document 1, we navigate to details view and render data for document 1.
User navigates back to list of documents.
User taps document 2, we navigate to details view
document 2 throws error, react-apollo shows data for document 1!!!????

why would we show data for document 1? Document 1 has nothing to do with anything. Why would we ever want to do that? These are two totally separate queries

Additionally, I find the current "workaround" unacceptable. We ran into this issue because it happened in production on one of our queries. Are we supposed to code defensively for all of our queries? For something that makes absolutely no sense to happen in the first place? This issue should be reopened, it's valid and not solved.

thanks!

[sfmayke]

@helfer
i had the same problem, my Query was returning old data along with the error, the way i workaround that was by simply setting the object (data) to null in the onError variable in the useQuery hook, by doing that every time a got a error from the query, the old data was cleaned.