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.

Sign up for GitHub

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

Compare DocumentNode used in refetchQueries as strings #12236

Merged
merged 16 commits into from
Dec 19, 2024
+326 −21
Merged

Compare DocumentNode used in refetchQueries as strings #12236

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d04679f
Add test for `refetchQueries` with a `DocumentNode`
charpeni Dec 18, 2024
19d14a7
Test different references of a same query `DocumentNode`
charpeni Dec 18, 2024
9f4b665
Compare `DocumentNode` as strings via `print`
charpeni Dec 18, 2024
521b9d1
Add changeset
charpeni Dec 18, 2024
71f4652
Run Prettier
charpeni Dec 18, 2024
ac07d25
Set changeset as a patch
charpeni Dec 18, 2024
75c3420
Simplify changeset
charpeni Dec 18, 2024
3c10ea5
Remove `legacyOneTimeQuery` reference
charpeni Dec 19, 2024
7873f3c
Remove `legacyOneTimeQuery` reference
charpeni Dec 19, 2024
8860968
Only show query name and support anonymous queries
charpeni Dec 19, 2024
1dfb266
Update `.api-reports`
charpeni Dec 19, 2024
b632d5d
Fix grammar for anonymous query
charpeni Dec 19, 2024
af15100
Fix anonymous query test
charpeni Dec 19, 2024
17c8056
Simplify the logic to get the query name
charpeni Dec 19, 2024
95cd778
Update `.api-reports`
charpeni Dec 19, 2024
03334ab
Update size limits
jerelmiller Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/gorgeous-sheep-knock.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix an issue with `refetchQueries` where comparing `DocumentNode`s internally by references could lead to an unknown query, even though the `DocumentNode` was indeed an active query—with a different reference.
43 changes: 28 additions & 15 deletions src/core/QueryManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { invariant, newInvariantError } from "../utilities/globals/index.js";

import { parse } from "graphql";
import type { DocumentNode } from "graphql";
// TODO(brian): A hack until this issue is resolved (https://github.com/graphql/graphql-js/issues/3356)
type OperationTypeNode = any;
Expand Down Expand Up @@ -899,15 +900,15 @@ export class QueryManager<TStore> {
include: InternalRefetchQueriesInclude = "active"
) {
const queries = new Map<string, ObservableQuery<any>>();
const queryNamesAndDocs = new Map<string | DocumentNode, boolean>();
const queryNamesAndQueryStrings = new Map<string, boolean>();
const legacyQueryOptions = new Set<QueryOptions>();

if (Array.isArray(include)) {
include.forEach((desc) => {
if (typeof desc === "string") {
queryNamesAndDocs.set(desc, false);
queryNamesAndQueryStrings.set(desc, false);
} else if (isDocumentNode(desc)) {
queryNamesAndDocs.set(this.transform(desc), false);
queryNamesAndQueryStrings.set(print(this.transform(desc)), false);
} else if (isNonNullObject(desc) && desc.query) {
legacyQueryOptions.add(desc);
}
Expand Down Expand Up @@ -935,12 +936,12 @@ export class QueryManager<TStore> {

if (
include === "active" ||
(queryName && queryNamesAndDocs.has(queryName)) ||
(document && queryNamesAndDocs.has(document))
(queryName && queryNamesAndQueryStrings.has(queryName)) ||
(document && queryNamesAndQueryStrings.has(print(document)))
) {
queries.set(queryId, oq);
if (queryName) queryNamesAndDocs.set(queryName, true);
if (document) queryNamesAndDocs.set(document, true);
if (queryName) queryNamesAndQueryStrings.set(queryName, true);
if (document) queryNamesAndQueryStrings.set(print(document), true);
}
}
});
Expand Down Expand Up @@ -969,15 +970,27 @@ export class QueryManager<TStore> {
});
}

if (__DEV__ && queryNamesAndDocs.size) {
queryNamesAndDocs.forEach((included, nameOrDoc) => {
if (__DEV__ && queryNamesAndQueryStrings.size) {
queryNamesAndQueryStrings.forEach((included, nameOrQueryString) => {
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
);
const isQueryString =
nameOrQueryString.startsWith("query ") ||
nameOrQueryString.startsWith("{"); // Shorthand anonymous queries
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not fully happy with how it ended up here.

At this stage, nameOrQueryString is always a string, but we need to know whether it's a query name, or a printed document node. I'm not sure if we wanted to use AST utils here to parse the string and try to capture the operation node out of it, but it seems like it could be expensive, and they are built with invariants, too. I opted to check if it starts with either query or { (a shorthand for an anonymous query starts with a selection set).

Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

It actually might be easiest to just add another Map where the key is the stringified query and the value is the query name (or null in the case that its anonymous), that way we can skip the parsing step and just add it where we know its a DocumentNode on line 910.

const queryNames = new Map<string, string | null>();

// line 910
} else if (isDocumentNode(desc)) {
  const queryString = print(this.transform(desc));
  queryNames.set(queryString, getOperationName(desc));
  queryNamesAndQueryStrings.set(queryString, false);
}

Then getting the query name would be as simple as:

const queryName = queryNames.get(nameOrQueryString);

In fact, it might also be best just to put the case where you're passing query strings in there as well, then you wouldn't need the isQueryString check:

// line 909
if (typeof desc === "string") {
  queryNames.set(desc, desc);
  queryNamesAndQueryStrings.set(desc, false);

This way queryNames.get(...) will work in both cases. This at least skips the need to parse again.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea! I was on the fence between that or turning the actual map values into an object: { included, queryName }, but I think it's probably better to split those concerns for now.

Will do those changes, thank you!

const queryName =
isQueryString ?
getOperationName(parse(nameOrQueryString))
: nameOrQueryString;

if (queryName) {
invariant.warn(
`Unknown query named "%s" requested in refetchQueries options.include array`,
queryName
);
} else {
invariant.warn(
`Unknown query anonymous requested in refetchQueries options.include array`
charpeni marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
});
}
Expand Down
Loading
Loading