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

[Docs] refetchQueries can fetch non active queries #11894

Open
maon-fp opened this issue Jun 13, 2024 · 8 comments
Open

[Docs] refetchQueries can fetch non active queries #11894

maon-fp opened this issue Jun 13, 2024 · 8 comments

Comments

@maon-fp
Copy link

maon-fp commented Jun 13, 2024

Issue Description

In the docs it states:

You can only refetch active queries.

But one can pass an arbitrary query to the refetchQueries and apollo client will query and cache the results.

So either the docs are not correct or the client is "overfetching" not active queries.

Link to Reproduction

https://codesandbox.io/p/devbox/busy-maria-gjhpgz

Reproduction Steps

Go to the sandbox. Open the devtools. Add a person twice and you'll see the refetched animals in the console.

@apollo/client version

3.10.4

@jerelmiller
Copy link
Member

Hey @maon-fp 👋

It looks like you're using the object-based syntax in refetchQueries, which does allow you to pass arbitrary queries to refetchQueries. This is currently undocumented and listed as "legacy" in our codebase though, so I believe this is a discouraged pattern (though to be honest, I'm not sure the historical reasons why).

} else if (isNonNullObject(desc) && desc.query) {
legacyQueryOptions.add(desc);
}

The "active" queries refer to the DocumentNode and string-based inputs. I'll see if I can dig up any information on the historical reason why this is "legacy" since this change was made before my time on the team.

@maon-fp
Copy link
Author

maon-fp commented Jun 14, 2024

Thanks for your answer. I was just irritated by the docs not matching my observed behavior.

} else if (isNonNullObject(desc) && desc.query) {
legacyQueryOptions.add(desc);
}

The "active" queries refer to the DocumentNode and string-based inputs. I'll see if I can dig up any information on the historical reason why this is "legacy" since this change was made before my time on the team.

As far as I understand the DocumentNode I'm using one in my example:

const ALL_ANIMALS = gql`
  query AllAnimals {
    animals {
      id
      name
    }
  }
`;

@Cellule
Copy link

Cellule commented Jun 28, 2024

I dug a bit into this and the problem is that the code here doesn't exclude "inactive" queries when trying to refetch queries by name

if (
fetchPolicy === "standby" ||
(include === "active" && !oq.hasObservers())
) {
return;
}

It should likely instead be the following

        if (
          fetchPolicy === "standby" ||
-         (include === "active" && !oq.hasObservers())
+         (include !== "all" && !oq.hasObservers())
        ) {
          return;
        }

That way if the query is "inactive" (aka has no observers) then it would not be included in the refetch

However, if the query is not found it would warn later that it didn't it. Which is questionable

if (__DEV__ && queryNamesAndDocs.size) {
queryNamesAndDocs.forEach((included, nameOrDoc) => {
if (!included) {
invariant.warn(
typeof nameOrDoc === "string" ?
`Unknown query named "%s" requested in refetchQueries options.include array`
: `Unknown query %o requested in refetchQueries options.include array`,
nameOrDoc
);
}
});
}

In my case I often mean "refetch IF it active, otherwise, meh"

@jerelmiller
Copy link
Member

@maon-fp apologies, coming back to this, I realize I misread your reproduction. You're right that the DocumentNode case should only work with active queries.

Good find on the code @Cellule. You're right, technically an inactive query won't be excluded if you're passing the query by name.

To be totally honest though, I'm not entirely sure the case where a query can become inactive and participate in the refetch, mostly because as soon as an ObservableQuery no longer has observers, we tear down the ObservableQuery

if (this.observers.delete(observer) && !this.observers.size) {
this.tearDownQuery();
}

which calls stopQuery

this.queryManager.stopQuery(this.queryId);

which ultimately ends up removing that query from the list of queries (see line 1068):

public stopQuery(queryId: string) {
this.stopQueryNoBroadcast(queryId);
this.broadcastQueries();
}
private stopQueryNoBroadcast(queryId: string) {
this.stopQueryInStoreNoBroadcast(queryId);
this.removeQuery(queryId);
}
public removeQuery(queryId: string) {
// teardown all links
// Both `QueryManager.fetchRequest` and `QueryManager.query` create separate promises
// that each add their reject functions to fetchCancelFns.
// A query created with `QueryManager.query()` could trigger a `QueryManager.fetchRequest`.
// The same queryId could have two rejection fns for two promises
this.fetchCancelFns.delete(queryId);
if (this.queries.has(queryId)) {
this.getQuery(queryId).stop();
this.queries.delete(queryId);
}
}

This Map of queries is the same one that refetchQueries uses to compare against:

this.queries.forEach(({ observableQuery: oq, document }, queryId) => {

So theoretically is should only be active queries it can use since those are the only ones that should be available in that Map of queries.

@maon-fp I tried your reproduction again and wasn't able to see the animals query fetched after the mutation. Each time I run the mutation, I'm seeing the warning about not being able to find a query that matches that document node. Is there something I'm missing here? I'd love to be wrong here and understand if there is a case where an inactive query can be refetched so we can update the docs accordingly. Let me know what I can do to see that fetch happen!

@Cellule
Copy link

Cellule commented Jun 28, 2024

I can guarantee there are inactive queries left in that list.
See #11914 for a related issue with a repro

@levrik
Copy link
Contributor

levrik commented Dec 16, 2024

I'm also seeing the same issue that inactive queries are getting refetched when just passing a DocumentNode to refetchQueries. Calling client.getObservableQueries() doesn't include these queries that still get refetched. The queries are still present in client.queryManager.queries though.

@levrik
Copy link
Contributor

levrik commented Dec 16, 2024

After digging a bit deeper I realized that my queries are duplicate in queryManager.queries for some reason. Queries get correctly removed by queryManager.stopQuery() but these duplicates aren't.

@levrik
Copy link
Contributor

levrik commented Dec 16, 2024

I think what I'm seeing is #9903

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

No branches or pull requests

4 participants