Fix refetchQueries context

[jonaskello]

Fixes #3576

[jonaskello]

Let me know if there is something more I need to fix :-).

[jonaskello]

Btw, this is currently a blocking issue for me so would be nice to have it released in a patch version.

[horva]

I kind of feel better solution would be:

  this.query({
    query: refetchQuery.query,
    variables: refetchQuery.variables,
    fetchPolicy: 'network-only',
+   context: refetchQuery.context
  });

where we explicitly state which context to use.
Mutation and refetch query don't have to share same context.
In our project, we are using context to specify API version.
As we are slowly transitioning to API v2 mutation is still executed on API v1 while refetch query uses API v2

[jonaskello]

Yes, perhaps it should be possible to use separate context for the mutation and the refetchQueries.

However I think the expected default behavior would be that the context specified in the component is used for all requests that that component makes. At the very least, using the same context is better than using no context at all for refetchQueries and it is an easy fix.

Then a way to override the default behavior and use separate contexts could be added in another PR, because I think that would require more discussion.

[horva]

Then context: refetchQuery.context || context would fit us both, meaning Use default unless context explicitly specified.

[horva]

One more thing that I noticed was that previous version of the package reused same context as cached query when using string value for refetchQueries. Now that is gone. But this could also be somehow connected to react-apollo changes. Didn't find which change affected this feature yet.

[jonaskello]

I agree on that, but I don't think context: refetchQuery.context || context is simple to implement since refetchQuery today is of type PureQueryOptions and it does not have a context property.

The fix is in this PR is very simple, it requires a change to a single line of code, and I think it could just be a patch version bump. It was implemented as suggested by @hwillson in #3576 comment.

Adding a new feature to handle contexts separately for each refetchquery probably requires a minor version bump, and I think that will be eventually covered by #2009.

[hwillson]

Hi all - I've been digging through this further, and think we might need to change direction here.

refetchQueries is intended to be used to refetch queries that have already been fetched once, which means they might have used their own context the first time they were fetched. If we update mutate such that the specified mutate context overrides all of its refetchQueries contexts, then we'll be clobbering the initial query context that was used to fetch the query the first time. Since the query is an Observable, any others parts of an application that were relying on that query using its initial context, will now get data back using the mutate specific context, instead of the original context. We could consider making a refetchQueries context override coming from a mutate call temporary; so the refetched query would use the new context to get data, but the ObservableQuery itself would not be updated to permanently use the new context (it would retain its previous context). Doing this could get a bit messy though; we now have a named query that could be refetched in multiple parts of an application, but could be fetched differently due to varying context's.

Another option is to consider only overriding the refetchQueries query context if the queries to be refetched don't have an existing context. This could also be problematic if other parts of the application are already relying on the fact that the ObservableQuery was previously fetched without a context. Adding one now could mess up other parts of the app.

Adding a bit more insult to injury - updating the mutate context option to impact refetchQueries could be a breaking change. Current applications might be relying on the fact that mutate's context will not affect any refetched queries. We could work around this by adding a new mutate option like overrideRefetchedQueryContext that is false by default, but that might be a bit confusing to people.

For now, I think the best way forward here might be to handle this through documentation. We could update the mutate docs to:

1. Explicity state that the context option is only used for the mutation itself. It won't be used for refetchQueries. By default, refetched queries will use whatever context was in place when they were first created.

2. Mention that if a custom context is needed for a specific refetched query, then make sure that context is in place when the query is first called/created. A query that was initialized with a specific context will retain that context as part of its ObservableQuery instance. Then when mutate triggers its refetchQueries, the ObservableQuery context will be passed though the defined Apollo Link.

@jonaskello @horva Any objections to this approach? Additionally I'll flag this for reconsideration as part of the Apollo Client 3.x release. Thanks!

[jonaskello]

@hwillson In the end I went a different route without apollo since I needed to solve this. However I have some comments:

1. IIRC, the queries do not use their original context when being refetched, they use no context at all. So that might be a bug then?

2. If context is persisted and coupled to each query there might be a problem for the use-case of authorization headers. I'm thinking about when you have a token that needs to be refreshed. If the token is refreshed, but the queries uses old contexts with old authorisation headers, and thus old tokens that could be a problem. I guess the question is if the context is part of the query, or part of each request for the query. I would say that at least the headers should be part of each request, since you might want to change the headers between requests in order to have a fresh token.

[hwillson]

Thanks for the feedback @jonaskello. Regarding 1 - they should use their original context. I'll be adding a test to this PR that demonstrates this. Regarding 2 - right now the context is stored as part of the ObsearvableQuery instance itself. It's then re-used whenever a refetch happens. Yes, there is always potential for the stored context to be out of date when a query refetches, but application level exception handling can be put in place to handle this. It's not a 100% foolproof, but it's better than the other options available currently.

Okay - I'm going to update this PR to cover the changes I discussed earlier, and get it merged (that way you'll still get the contribution credit 👍). Thanks for your contribution and time!