Apollo-client: [Bug] `loading` doesn't change back to false if updateQuery doesn't mutate the store

[migueloller]

I was dealing with pagination in an app and I forgot to update my hasNextPage field which caused a call to fetchMore which returned an empty array (because the connection had no more edges). When I proceeded to merge this with my previous result, the loading prop didn't change to false after the APOLLO_UPDATE_QUERY_RESULT was fired.

I tried to reproduce the error by simply returning previousResult from updateQuery. Funnily enough, the error was only happening when I included pagination variables. This leads me to believe that the bug only occurs when APOLLO_QUERY_RESULT changes but APOLLO_QUERY_UPDATE_RESULT doesn't update the store.

Here is a snippet of my code (I changed some variable names for clarity):

// repro
fetchMore({
  variables: { after: connection.pageInfo.endCursor },
  updateQuery: (previousResult, { fetchMoreResult: { data } }) => previousResult,
});

If variables is removed (or after), the bug will stop occurring. If after is removed, but first is added (thus changing APOLLO_QUERY_RESULT but NOT APOLLO_UPDATE_QUERY_RESULT) then the bug will start occurring again.

tl;dr: If updateQuery results in a different query being fetched but it returns the previous result, loading will never be set to false even after both APOLLO_QUERY_RESULT and APOLLO_QUERY_UPDATE have been fired.

EDIT: FYI, our GraphQL backend uses the Relay Cursor Connections Specification for pagination.

[helfer]

Thanks, that sounds like it could be a bug. We'll probably take care of this when we refactor the loading state in the next few weeks.

cc @tmeasday

[migueloller]

@helfer, maybe this is a topic for a separate issue, but are there plans to change the way the internal store works so that it doesn't break time travel? Right now dispatching an APOLLO_QUERY_INIT action will actually send the request, this will wreak havoc when trying to time travel. Ideally, dispatching that action wouldn't send a request, but vice versa, when the request is sent, that action should be dispatched.

[tmeasday]

@migueloller just to be clear here -- are you using react-apollo? Because if I understand things correctly, AC doesn't set loading: true, it's RA that does that. If so, strictly speaking this is a RA bug.

OTOH I would have thought RA handles this case, via this line: https://github.com/apollostack/react-apollo/blob/master/src/graphql.tsx#L515 (although... maybe the problem is that result here is the result of the fetchMore query, not the merged results from updateQuery).

I'm mid-process of completely refactoring this code path, so I think the bug will (hopefully) be fixed along the way, but if you have the time, it'd be really really amazing if you could put a PR together to RA with a failing test case. That would ensure I do actually fix it.

[tmeasday]

@helfer, maybe this is a topic for a separate issue, but are there plans to change the way the internal store works so that it doesn't break time travel? Right now dispatching an APOLLO_QUERY_INIT action will actually send the request, this will wreak havoc when trying to time travel. Ideally, dispatching that action wouldn't send a request, but vice versa, when the request is sent, that action should be dispatched.

Probably we should take this to a separate issue, but this is surprising (to me anyway). In my reading of the code, I wouldn't expect that to happen. The QueryManager dispatches the APOLLO_QUERY_INIT right before sending off the request.

What is the behaviour you are seeing? Maybe it's best to open a separate bug about it.

[migueloller]

@tmeasday, I am using react-apollo. Sorry for the misunderstanding.

I've split the issue into two separate ones as needed. Closing this one.