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

Compare DocumentNode used in refetchQueries as strings #12236

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": minor
charpeni marked this conversation as resolved.
Show resolved Hide resolved
---

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. They are now compared as strings.
charpeni marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 5 additions & 7 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -899,15 +899,15 @@ export class QueryManager<TStore> {
include: InternalRefetchQueriesInclude = "active"
) {
const queries = new Map<string, ObservableQuery<any>>();
const queryNamesAndDocs = new Map<string | DocumentNode, boolean>();
const queryNamesAndDocs = new Map<string, boolean>();
const legacyQueryOptions = new Set<QueryOptions>();

if (Array.isArray(include)) {
include.forEach((desc) => {
if (typeof desc === "string") {
queryNamesAndDocs.set(desc, false);
} else if (isDocumentNode(desc)) {
queryNamesAndDocs.set(this.transform(desc), false);
queryNamesAndDocs.set(print(this.transform(desc)), false);
} else if (isNonNullObject(desc) && desc.query) {
legacyQueryOptions.add(desc);
}
Expand Down Expand Up @@ -936,11 +936,11 @@ export class QueryManager<TStore> {
if (
include === "active" ||
(queryName && queryNamesAndDocs.has(queryName)) ||
(document && queryNamesAndDocs.has(document))
(document && queryNamesAndDocs.has(print(document)))
) {
queries.set(queryId, oq);
if (queryName) queryNamesAndDocs.set(queryName, true);
if (document) queryNamesAndDocs.set(document, true);
if (document) queryNamesAndDocs.set(print(document), true);
}
}
});
Expand Down Expand Up @@ -973,9 +973,7 @@ export class QueryManager<TStore> {
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`,
`Unknown query %s requested in refetchQueries options.include array`,
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 wasn't sure what to do here.

We don't have a way to differentiate query name from a DocumentNode anymore, so we can either always say Unknown query, check if nameOrDoc starts with query (<- the trailing space is important), or have the map holding an object instead of only a boolean: { type, included }.

Copy link
Member

@jerelmiller jerelmiller Dec 18, 2024

Choose a reason for hiding this comment

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

Its probably a good idea to add a fallback name here that identifies it as anonymous, otherwise this might get unreadable for large queries. Here is a quick test I did with this change if you pass an unrecognized document node:

Unknown query query fakeQuery {                                                           
  fake                                                                                                                                                                                  
} requested in refetchQueries options.include array

Even with this being a single field query, its already a bit unreadable.

That said, the old message wasn't much better 😂

Unknown query {                                                                           
  kind: 'Document',                                                                                                                                                                     
  definitions: [                                                                          
    {                                                                                                                                                                                   
      kind: 'OperationDefinition',                                                                                                                                                      
      operation: 'query',                                                                                                                                                               
      name: { kind: 'Name', value: 'fakeQuery' },                                                                                                                                       
      variableDefinitions: [ [length]: 0 ],                                               
      directives: [ [length]: 0 ],                                                                                                                                                      
      selectionSet: { kind: 'SelectionSet', selections: [ [Object], [length]: 1 ] }                                                                                                     
    },                                                                                    
    [length]: 1                                                                                                                                                                         
  ],                                                                                                                                                                                    
  loc: Location {                                                                                                                                                                       
    start: 0,                                                                                                                                                                           
    end: 66,                                                                                                                                                                            
    source: Source {                                                                                                                                                                    
      body: '\n          query fakeQuery {\n            fake\n          }\n        ',                                                                                                   
      name: 'GraphQL request',                                                            
      locationOffset: { line: 1, column: 1 },                                                                                                                                           
      [Symbol(Symbol.toStringTag)]: [Getter]                                                                                                                                            
    },                                                                                                                                                                                  
    [Symbol(Symbol.toStringTag)]: [Getter]                                                                                                                                              
  }                                                                                       
} requested in refetchQueries options.include array

Would you be willing to add an additional test case for an anonymous query document passed to refetchQueries that hits this condition? We probably should have had one all-along to prevent that monstrosity, but could be a good time to improve the error message while we're here.

I think using "anonymous" as the fallback name would be ok since we've used "anonymous" in other places that print out query strings. We've got a getOperationName utility function that can extract it from a document node that can help determine if it has a name.

Let me know what you think about that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To confirm the expectation, are you suggesting we keep only the branch Unknown query named "%s" requested for both cases? In other words, we never display the document node; we only show the query name or extract it from the document node via getOperationName.

It gets noisy quickly. In most cases, an operation name would be more helpful than the entire document node.

If so, could it be problematic for an unknown anonymous query? E.g.:

query {
  fake
}

Would produce:

Unknown query anonymous requested in refetchQueries options.include array

How do we debug this if the query is anonymous and we don't show the content?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the confusion! Yes I'm suggesting we display this:

Unknown query "fakeQuery" requested in refetchQueries options.include array

instead of this:

Unknown query query fakeQuery {                                                           
  fake                                                                                                                                                                                  
} requested in refetchQueries options.include array

for the error message, at least for named queries. I was thinking we'd do the same for anonymous queries and show your 2nd suggestion there:

Unknown query anonymous requested in refetchQueries options.include array

but I suppose showing the full query here isn't the end of the world.

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 believe you are correct.

Showing the full query only adds noise, and since warnings aren't shown by default, the URL leading to the error page is too long to be clickable. 😅

I've confirmed that the warning triggered by the invariant has a stack trace leading to the refetchQueries option. It should be enough to debug cases where we're tracking down an unknown query. In most cases, having the named query in a one-liner warning should be the optimal developer experience.

I just sent a commit refactoring the logic mentioned above and added missing tests for warnings, including the anonymous query.

Screenshot 2024-12-19 at 9 55 09 AM

nameOrDoc
);
}
Expand Down
167 changes: 165 additions & 2 deletions src/core/__tests__/QueryManager/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5075,7 +5075,7 @@ describe("QueryManager", () => {
(result) => {
expect(result.data).toEqual(secondReqData);
expect(consoleWarnSpy).toHaveBeenLastCalledWith(
'Unknown query named "%s" requested in refetchQueries options.include array',
"Unknown query %s requested in refetchQueries options.include array",
"fakeQuery"
);
}
Expand Down Expand Up @@ -5148,7 +5148,7 @@ describe("QueryManager", () => {
})
.then(() => {
expect(consoleWarnSpy).toHaveBeenLastCalledWith(
'Unknown query named "%s" requested in refetchQueries options.include array',
"Unknown query %s requested in refetchQueries options.include array",
"getAuthors"
);
})
Expand Down Expand Up @@ -5237,6 +5237,169 @@ describe("QueryManager", () => {
await expect(stream).not.toEmitAnything();
});

it("also works with a query document node", async () => {
const mutation = gql`
mutation changeAuthorName($id: ID!) {
changeAuthorName(newName: "Jack Smith", id: $id) {
firstName
lastName
}
}
`;
const mutationData = {
changeAuthorName: {
firstName: "Jack",
lastName: "Smith",
},
};
const query = gql`
query getAuthors($id: ID!) {
author(id: $id) {
firstName
lastName
}
}
`;
const data = {
author: {
firstName: "John",
lastName: "Smith",
},
};
const secondReqData = {
author: {
firstName: "Jane",
lastName: "Johnson",
},
};

const variables = { id: "1234" };
const mutationVariables = { id: "2345" };
const queryManager = mockQueryManager(
{
request: { query, variables },
result: { data },
delay: 10,
},
{
request: { query, variables },
result: { data: secondReqData },
delay: 100,
},
{
request: { query: mutation, variables: mutationVariables },
result: { data: mutationData },
delay: 10,
}
);
const observable = queryManager.watchQuery<any>({ query, variables });
const stream = new ObservableStream(observable);

await expect(stream).toEmitMatchedValue({ data });

await queryManager.mutate({
mutation,
variables: mutationVariables,
refetchQueries: [query],
});

await expect(stream).toEmitMatchedValue(
{ data: secondReqData },
{ timeout: 150 }
);
expect(observable.getCurrentResult().data).toEqual(secondReqData);

await wait(10);
Copy link
Member

Choose a reason for hiding this comment

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

Once we remove that check for legacyOneTimeQuery, we should be able to remove this as well since await expect(stream).not.toEmitAnything() will wait for 100ms on its own.


queryManager["queries"].forEach((_, queryId) => {
expect(queryId).not.toContain("legacyOneTimeQuery");
});
charpeni marked this conversation as resolved.
Show resolved Hide resolved

await expect(stream).not.toEmitAnything();
});

it("also works with different references of a same query document node", async () => {
const mutation = gql`
mutation changeAuthorName($id: ID!) {
changeAuthorName(newName: "Jack Smith", id: $id) {
firstName
lastName
}
}
`;
const mutationData = {
changeAuthorName: {
firstName: "Jack",
lastName: "Smith",
},
};
const query = gql`
query getAuthors($id: ID!) {
author(id: $id) {
firstName
lastName
}
}
`;
const data = {
author: {
firstName: "John",
lastName: "Smith",
},
};
const secondReqData = {
author: {
firstName: "Jane",
lastName: "Johnson",
},
};

const variables = { id: "1234" };
const mutationVariables = { id: "2345" };
const queryManager = mockQueryManager(
{
request: { query, variables },
result: { data },
delay: 10,
},
{
request: { query, variables },
result: { data: secondReqData },
delay: 100,
},
{
request: { query: mutation, variables: mutationVariables },
result: { data: mutationData },
delay: 10,
}
);
const observable = queryManager.watchQuery<any>({ query, variables });
const stream = new ObservableStream(observable);

await expect(stream).toEmitMatchedValue({ data });

await queryManager.mutate({
mutation,
variables: mutationVariables,
// spread the query into a new object to simulate multiple instances
refetchQueries: [{ ...query }],
});

await expect(stream).toEmitMatchedValue(
{ data: secondReqData },
{ timeout: 150 }
);
expect(observable.getCurrentResult().data).toEqual(secondReqData);

await wait(10);

queryManager["queries"].forEach((_, queryId) => {
expect(queryId).not.toContain("legacyOneTimeQuery");
});
charpeni marked this conversation as resolved.
Show resolved Hide resolved

await expect(stream).not.toEmitAnything();
});

itAsync(
"also works with a conditional function that returns false",
(resolve, reject) => {
Expand Down
Loading