Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add public method to retrieve all current observable queries #7813

Merged

Conversation

dannycochran
Copy link
Contributor

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests (covered by other tests that call this method implicitly).

This feature is to help create a workaround for this comment:

apollographql/apollo-feature-requests#272 (comment)

By exposing the current observable queries, consumers can look at all their "refetchQueries" for a mutation, and if any of them aren't in the result of "getObservableQueries()", they can figure out how to ensure those queries are re-fetched on subsequent mounts.

This logic was already being done by query manager for its own internal handling of "refetchQueries" logic, so we are just exposing a small piece of that here.

@brainkim brainkim self-requested a review June 1, 2021 22:09
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @brainkim, I was able to use the new getObservableQuery method in the implementation of client.refetchQueries, replacing the implementation-detail-heavy getQueryIdsForQueryDescriptor function.

I think this could use some more tests (especially the "all" and "active" functionality) (see 4e022bc), though the new code is pretty well exercised by the existing client.refetchQueries tests.

Over to @brainkim for the final review.

@benjamn benjamn force-pushed the getObservableQueries branch from 366eea9 to 4e022bc Compare June 2, 2021 22:20
@benjamn benjamn self-assigned this Jun 2, 2021
@benjamn
Copy link
Member

benjamn commented Jun 2, 2021

@dannycochran If the additional commits I pushed are working correctly, the only visible change compared to your original PR is that you can now pass an argument to client.getObservableQueries to specify whether it should return "all" queries, or only "active" queries, or a subset of explicitly included queries:

client.getObservableQueries("active") // default behavior
client.getObservableQueries("all") // include inactive queries too
client.getObservableQueries(["QueryName", QUERY_DOC, ...]) // also works!

That one small API change makes getObservableQueries suitable for use in the implementation of client.refetchQueries and client.reFetchObservableQueries, which explains most of my other changes outside ApolloClient.ts.

Please let me/us know if you have any thoughts/questions/reservations. We should be able to include this PR in the next RC release for you to try.

Comment on lines +576 to +580
public getObservableQueries(
include: RefetchQueriesInclude = "active",
): Map<string, ObservableQuery<any>> {
return this.queryManager.getObservableQueries(include);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, all "active" queries will be included, but you can pass any RefetchQueriesInclude-typed value (in other words, anything you can pass as options.include to client.refetchQueries).

Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving now that I've added more tests.

Comment on lines 765 to 770
if (fetchPolicy === "cache-only" ||
fetchPolicy === "standby" ||
!oq.hasObservers()) {
// Skip inactive queries unless include === "all".
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannycochran If we want to impose further restrictions on the set of active queries (for example, that we're watching the query rather than just sending it once), I think this would be the place to do it.

@benjamn benjamn force-pushed the getObservableQueries branch from c4089e2 to ba92852 Compare June 2, 2021 23:26
@brainkim brainkim self-requested a review June 4, 2021 14:43
benjamn added 3 commits June 4, 2021 11:09
…eries.

Queries with `fetchPolicy: cache-only` are certainly not "inactive" (you
just might not want to refetch them from the network), so excluding them
is a choice for the reFetchObservableQueries method to make.

Note: we are currently planning to deprecate reFetchObservableQueries
and remove it in Apollo Client v4, so this logic will disappear from the
codebase after that.

The new client.refetchQueries API provides a much more flexible way to
specify which queries you want to include, including the ability to
filter/skip them in arbitrary ways in the onQueryUpdated function.
Although passing one-time { query, variables } QueryOptions in the
options.include array is discouraged by the type system, it's still
allowed for the refetchQueries option for mutations, so we should make
sure to stop those temporary queries after they've been fetched.

I didn't want to complicate the client.getObservableQueries API to
return additional metadata about which queries are legacy/temporary, so
I'm using query ID strings (the keys of the getObservableQueries Map) to
convey that information.
@benjamn benjamn merged commit ff80d89 into apollographql:release-3.4 Jun 4, 2021
@benjamn
Copy link
Member

benjamn commented Jun 4, 2021

@dannycochran This should be available now in @apollo/[email protected], if you want to give it a try. Thanks for your ideas and implementation help!

@dannycochran
Copy link
Contributor Author

@benjamn @brainkim thanks for bringing the PR home! looking forward to 3.4

@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@lauriharpf
Copy link

After upgrading @apollo/client from 3.3.21 to 3.4.0, I noticed that some jest tests in our project that had previously passed silently started outputting warnings. For example:

lauriharpf@Lauris-MacBook-Pro my-mobile-app % yarn test MyComponent.test.tsx
yarn run v1.22.17
$ TZ="Europe/Helsinki" jest --maxWorkers=4 MyComponent.test.tsx
  console.warn
    Unknown query named "myGraphQLQuery" requested in refetchQueries options.include array

      at Function.warn (node_modules/@apollo/client/node_modules/ts-invariant/lib/invariant.esm.js:35:27)
      at node_modules/@apollo/client/core/QueryManager.js:444:42
          at Map.forEach (<anonymous>)
      at QueryManager.Object.<anonymous>.QueryManager.getObservableQueries (node_modules/@apollo/client/core/QueryManager.js:442:31)
      at QueryManager.Object.<anonymous>.QueryManager.refetchQueries (node_modules/@apollo/client/core/QueryManager.js:654:18)
      at QueryManager.Object.<anonymous>.QueryManager.markMutationResult (node_modules/@apollo/client/core/QueryManager.js:198:18)
      at node_modules/@apollo/client/core/QueryManager.js:105:49

The warnings seem to be introduced in this PR. The MyComponent under test refetches a query that is triggered elsewhere in the app. My understanding is that the warning is triggered because the test is limited to MyComponent and therefore myGraphQLQuery has never been run.

If the query is run by the code that is being tested, e.g. MyComponent is changed to do

export const MyComponent = () => {
  useMyGraphQLQuery({})
  // ...
}

then the warning is eliminated. I didn't notice this mentioned in the 3.4.0 changelog, but might have missed it.

Not sure what the best way to deal with a scenario like this is. We could change the implementation and eliminate the need for using refetchQueries via passing data back in the mutation response or clearing the stale items from the cache. Alternatively, we could change the test setup to run myGraphQLQuery, provided that we can guarantee that it has been run elsewhere in the app.

Just wanted to leave this here in case someone else happens to encounter the same thing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants