fetchPolicy set to cache-only after unmount

[jamiemccrindle]

Steps to Reproduce

I have a container that lists widgets and a another container that adds widgets. The add widget component is on a new page, so the list page is unmounted when the add widget runs. I'm using a saga to apply the mutation and then attempt to refetch the AllWidgets query. When I do, I get the following error:

Network error: cache-only fetchPolicy option should not be used together with query refetch.

Looking at other issues, it's not clear that this is buggy behaviour but it's also not clear how one goes about solving this problem. Any suggestions would be welcome.

More detail:

export default connect()(graphql(gql`
query AllWidgets {
    allWidgets { 
        nodes {
            id
            name
        }
    } 
}
`)(ListWidgets)))

const result = yield call(client.mutate.bind(client), {
    mutation: createWidgetMutation,
    variables: {name: name},
    refetchQueries: ['AllWidgets']
} as MutationOptions);

1. Create a component (see above)
2. Mount it
3. Unmount it
4. Run mutation that updates the query associated with the widget

Buggy Behavior

I get the following error:

Network error: cache-only fetchPolicy option should not be used together with query refetch.

Expected Behavior

The query is updated

Version

• [email protected]
• [email protected]

[cyrilepinat]

I have the same problem when I use refetchQueries options. It's working well without this option.

Could this be related to PR #531 ?

[jamiemccrindle]

Definitely. I suspect that just getting some guidance on the right lifecycle would be good here. As in, what's the right way to develop the following:

WidgetList is mounted
WidgetList queries for allWidgets
WidgetList is unmounted
AddWidget is mounted
AddWidget mutates by calling createWidget

[insert guidance here]

WidgetList is remounted and has the new widget in it's list of widgets, either by a new network call or by adding it clientside (supporting both would be great).

[blackxored]

I ran into this issue as well after a bit of tracing ended up here on the recycle method.
Any workarounds for this?

[cyrilepinat]

@blackxored for now my workaround is to remove refetchQueries option and use fetchPolicy: 'network-only' on the targeted query... this is not a long term solution of course 😄

[blackxored]

Yes, this works. But I hope this gets addressed soon. I'm using this on a modal like thing that animates and it's so sluggish because of it.

[cyrilepinat]

@blackxored you can still use updateQueries option that works great.

[blackxored]

Yeah I like that idea better, have done it since, thanks; however I miss the "for free" nature of refetchQueries in the meantime. Probably best practice is to use updateQueries though so I'm kind of on the fence on this one.

[jacobk]

@cyrilepinat Yes, this is caused by #531 indeed.

#531 was needed to make resetStore work sensibly for recycled queries but that's in direct conflict with refetching not currently active queries which is really important for cache invalidation/update purposes.

I keep coming back to that thinking that resetStore should be more strict and throw away all known queries, removing the need to balance recycled/active queries against each other. If the intended use case for resetStore is different it should not be recommended for e.g. log out flows and I'm not sure what it's utility is TBH.

[gforge]

Workaround solution help: Looked at the updateQueries docs and there is a lot of "magic" going on and it's not clear to me how to add this to a cursor pagination schema where the query also contains a total of X elements that should be incremented by 1. Is the direct access the correct way to go about this?

[zookatron]

I'm running into this issue as well. The fact that this was caused by upgrading from 1.0.0 to 1.0.1 is concerning. This kind of breakage of basic functionality shouldn't be happening in patch version bumps. New patch versions are only supposed to have backwards-compatible bug fixes. For me the best solution was to downgrade to 1.0.0 which is working just fine.

[helfer]

@jamiemccrindle Sorry I missed this issue thread until now. This is definitely a regression related to recycling of observables and #531, which were in turn attempts at fixing the problem with refetching or updating data related to queries that are not currently active. We discussed that issue at length late last year and concluded that the best way forward would be to not provide updating and refetching by name any more, and instead require specifying the query document and variables to refetch. Updating/refetching based on query documents now supported onupdateQueries and on refetchQueries, but we kept the old behavior in there for backwards compatibility (recycling queries was also an attempt at providing backwards compatibility).

I am more and more convinced that the right way to proceed is to start deprecating refetching and updating queries by name, but I'm curious to hear what other thoughts people have.

TLDR: Refetching queries by name isn't recommended any more. The workaround (and recommended practice going forward) is to provide a query strings/documents to refetchQueries as illustrated in the docs.

I'm going to see what I can do to fix this regression in one of the next versions of React Apollo, but I'd also recommend that folks make the necessary updates to do refetching via documents instead of query names.

If you have any thoughts on the matter, this thread is probably the best place to have a discussion! We're always open to ideas for how to make every aspect of Apollo better, and we especially like pull requests! 🙂

[jacobk]

@helfer my concern with removing refetching by name is that some queries have complex variable setups not easily provided from other components e.g. lists with pagination, sorting, filtering etc.

Refetching such queries by name is a simple solution to make sure they are not stale since updateQueries might not be feasible if the sorting (etc) logic is complex.

This is related to the very good discussion in apollographql/apollo-client#621 and might be worth considering for react-apollo to have a pre-built solution for.

Maybe an invalidateQueries option to mutate or similar is a good trade off to lazily refetch queries which shouldn't impact the query recycling.

(recycling queries was also an attempt at providing backwards compatibility).

Isn't that needed to make sure updateQueries works too? Which is still a recommended (not to be deprecated) API?

[jamiemccrindle]

Thanks all! @helfer I'm still not 100% sure what the right solution for this is:

1. WidgetList is mounted
2. WidgetList queries for allWidgets
3. WidgetList is unmounted
4. AddWidget is mounted
5. AddWidget mutates by calling createWidget
6. WidgetList is mounted
7. WidgetList queries for allWidgets

Do I call the updateQueries in step 5? Will this mean that when the list is remounted in step 6 that it will have up to date data because it's executing the same query as 5?

[gforge]

@helfer - using updateQueries is most likely the most efficient approach, but the logic may be rather complicated and perhaps nothing that has to be ready in a prototype. Having a quick & dirty "re-fetch everything" option is therefore in my min motivated, although it could be accompanied with a user warning and recommendation towards using a more efficient solution.

I've been thinking about my previous question and the more I think about it, the trickier the problem becomes. It seems like the server needs to return (apart from the object itself):

• the new object's cursor
• new total

We then need to rewrite any filters/sort in the client in order to position it according to the current pagination/sort/filter settings. The traditional comment example the new item is simply appended to the end but anything more complicated seems like a non-trivial task, or am I missing something?

[helfer]

Hey folks, I have some good news (if it works). I've prepared a PR to apollo-client that would fix this problem (and maybe some others as well). The gist is that instead of co-opting the "cache-only" fetchPolicy for recycled queries, we explicitly mark them as "standby". That means they don't run during store reset, but you can use them in refetch and updateQueries. It also means that we'd no longer waste CPU cycles recomputing results that nobody cares about. Here's the PR: #519

@jacobk @jamiemccrindle @gforge would one of you have time to make a PR to make the necessary changes in react-apollo to use the standby fetchPolicy for recycled queries once the PR is merged into Apollo Client? You could start the PR even before we merge the original PR into apollo-cleint, because step 1 should be to write a failing test for your current use-case: calling refetchQueries with a recycled query.

@jacobk I'm actually not a huge fan of updateQueries by name either. My life would be a lot easier if we never had to keep track of queries by name inside of Apollo Client. 😄 But the goal of Apollo Client is to make your life easier, not mine, so I do see the need and usefulness of refetchQueries and updateQueries.

The main challenge with them is that they complicate the logic (number of possible states) in Apollo Client by quite a lot. It's pretty hard to ensure that they work well, especially in some corner cases, which is why I'd say that if you want to be totally safe you should use update for now, where possible. If you have a use-case where update would be really hard to apply in comparison, please write that down here: #519 !!

@jamiemccrindle yeah, I think in that case you should call update in step 5, or you can call refetchQueries, but provide a query document instead of a name. That's what I'd recommend doing at the moment.

@gforge I was going to say that you could just use the network-only fetchPolicy, but obviously that doesn't work if your view is shown at the same time. However, if the component whose data you want to refetch isn't shown on the same page, network-only probably gets you pretty far.

Anyway, I think I'll go back to enjoying my Sunday now. If the PR on apollo-client works as intended, then maybe there's nothing you guys need to do, and it will just work again like before (minus the bugs due to query recycling).

[jacobk]

@helfer I've started to write a failing test case but it seems a bit tricky since the rejected promise from https://github.com/apollographql/apollo-client/blob/master/src/core/ObservableQuery.ts#L203 ends up being swallowed inside the client https://github.com/apollographql/apollo-client/blob/master/src/core/QueryManager.ts#L331 and escapes the test-harness.

Any ideas about dealing with unhandled rejections in test cases? I'll have another look later when I have time.

I've pushed pushed by progress this far here if somebody want to give it a go master...jacobk:standy-recycled-queries

[jacobk]

I've got an initial breaking test added to #671 now. It still needs some tweaking (and I'm still concerned about not being able to detect rejection, see above) but the test catches this issue and seems to pass fine with #531 reverted.

[cyrilepinat]

@helfer sorry for the late answer, I've just tested the v1.2.0 of react-apollo and it seems that refetchQueries by name has been actually removed as you said right ? For my case, I came to refetchQueries after facing problems using updateQueries with complex variables that I cannot retrieve when I need to update list. So refetchQueries by name was my very last hope to not use fetchPolicy: 'network-only' and this is not available anymore 😢 haha so I don't know what to do now 😄

[helfer]

@cyrilepinat the functionality to refetch by name is still available, we just don't recommend using it in the longer term any more, because we might deprecate it in the future, or split it out into a separate package if possible.

[cyrilepinat]

@helfer oh ok I see, the query is actually refetched but the component is not rerender and keeps old data...

[helfer]

@cyrilepinat in that case I think it's a different issue. Please open a new issue and provide a reproduction there! 🙂