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

Clarify active query limitation for refetchQueries #11357

Conversation

nopelluhh
Copy link
Contributor

@nopelluhh nopelluhh commented Nov 9, 2023

Context

The refetchQueries mutation option only works for active queries when referenced by operation name or document node. Based on @jerelmiller's mention of the [{ query: QUERY, variables: {} }] approach being considered "legacy", it seems like updating the docs to clarify the cause of the unknown query console warning would be helpful (at least to me after I spent a bit too long trying to figure out why my query wasn't refetching!)

Changes

  • Clarify the limitation of only active queries being refetchable in the docs

Callouts

I didn't see any particular definition of active queries anywhere in the documentation, so I tried to provide a simple "on the current page" version. It does seem suspicious that this behavior might be masked in development + strict mode but I'm not sure if that detail is relevant.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • 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

@nopelluhh nopelluhh requested a review from a team as a code owner November 9, 2023 23:54
@apollo-cla
Copy link

@nopelluhh: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link

netlify bot commented Nov 9, 2023

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 5f21a3d

Copy link

changeset-bot bot commented Nov 9, 2023

⚠️ No Changeset found

Latest commit: 5f21a3d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

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

Thanks for updating the docs to help others avoid the same issue you faced @nopelluhh ! 🙏

I've included a suggestion about placement and wording. I'd love @jerelmiller 's 👀 for a technical accuracy check!

@@ -223,6 +223,8 @@ You can provide the `refetchQueries` option either to `useMutation` or to the mu

Note that in an app with tens or hundreds of different queries, it can be challenging to determine exactly which queries to refetch after a particular mutation.

> Only active queries can be refetched using this option. If the query you want to refetch is not used by a component on the current page, you may want to consider modifying your mutation to support updating the cache directly as described below.
Copy link
Contributor

Choose a reason for hiding this comment

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

To save other folks from trying to refetch inactive queries, it would be best to place this callout right at the top of the section, under the heading ## Refetching queries. I would also rewrite it to be explicit about the definition of "active queries."

Suggested change
> Only active queries can be refetched using this option. If the query you want to refetch is not used by a component on the current page, you may want to consider modifying your mutation to support updating the cache directly as described below.
> You can only refetch _active_ queries. Active queries are those used by components on the current page. If the data you want to update is not fetched by a component on the current page, it's best to [update your cache directly](#updating-the-cache-directly).

@jerelmiller , would you be able to verify that this is an accurate definition for "active queries"? Like @nopelluhh , I found it used on this page but without a clear definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @Meschreiber - I've updated the wording to your suggestion and moved up directly under the code example. Happy to move all the way to the top if you'd prefer, but I was wary of introducing a potential limitation before explaining the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies for the delay on this, a lot of the team has been at a conferences as of late.
This placement breaks up the code sample and the explanation of the code sample, so I would prefer to bump it back down, or back up, depending on how relevant it is to readers. I agree that it's best to place potential limitations lower, but am inclined to put more absolute disqualifies higher up.

@jerelmiller , when you get a chance, would you be able to confirm the definition of "active queries" (those used by components on the current page?) and whether refetchQueries can only be used by such queries? That'll help us figure out where this disclaimer belongs. :)

Copy link
Member

Choose a reason for hiding this comment

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

Hey @nopelluhh!! So sorry I've been so slow to respond and review. I was speaking at 2 conferences this week so I've let GitHub reviews slip through the cracks a bit. I have this on my list to review for accuracy next week. Thanks for the patience!

Copy link

@pfurmaniak pfurmaniak Nov 19, 2023

Choose a reason for hiding this comment

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

Hey guys, this is a really good change as I've spent too much time than I'd care to admit figuring this one out.

@Meschreiber @jerelmiller is there a possibility to implement a functionality that will refetch inactive queries as soon as they become active so that this behavior is more straightforward? Because of lack of this feature I've implemented my own mechanism to do so, but I bet I'm not the only one who'd think that should be the default behavior.

To clarify: I appreciate the fact that in a big, enterprise-grade application it will be very hard to determine which queries need refeching. However, in smaller applications this kind of behavior would streamline the development massively - I think. Let me know your opinion.

@nopelluhh Great PR anyway - thanks a lot.

Many thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point re: breaking up the example @Meschreiber, will wait for @jerelmiller's review before moving around again. And no prob on the delay @jerelmiller, the docs will survive another day. I'm OOO next week so take your time/feel free to adjust as you see fit.

Copy link
Member

Choose a reason for hiding this comment

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

@pfurmaniak definitely an interesting suggestion! Would you mind opening a feature request in our feature requests repo so we can track that separately? No guarantee we will add/implement something exactly like that, but it gives us the ability to ensure we don't forget that type of use case when we revisit refetchQueries in the future. Thanks!

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

🎉 This change looks great to me. Thanks so much for the contribution @nopelluhh!

@jerelmiller jerelmiller merged commit bd4dd34 into apollographql:main Nov 20, 2023
22 of 23 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants