Compare `DocumentNode` used in `refetchQueries` as strings #12236

charpeni · 2024-12-18T19:28:34Z

We have the ability to pass a DocumentNode directly to refetchQueries, but unfortunately, it doesn't always work as expected. DocumentNode are compared by references as they are added as an object to the Map. See the issue mentioned above (#12164) for a detailed investigation.

To prevent active queries from being marked as unknown by Apollo Client, we can compare DocumentNodes by value rather than reference. To do so, we can leverage the internal print function (thanks @jerelmiller!).

netlify · 2024-12-18T19:28:37Z

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

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`03334ab`

svc-apollo-docs · 2024-12-18T19:28:38Z

✅ Docs Preview Ready

No new or changed pages found.

pkg-pr-new · 2024-12-18T19:29:34Z

npm i https://pkg.pr.new/@apollo/client@12236

commit: 03334ab

changeset-bot · 2024-12-18T19:35:45Z

🦋 Changeset detected

Latest commit: 03334ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

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

jerelmiller · 2024-12-18T20:36:57Z

Don't worry about the failing size report. I'll make sure that gets updated before this gets merged 🙂

charpeni · 2024-12-18T20:43:34Z

src/core/QueryManager.ts

-            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`,


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 }.

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!

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?

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.

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.

charpeni · 2024-12-18T20:47:26Z

Thank you!

I just confirmed that running this pull request (via https://pkg.pr.new/@apollo/client@12236) within our application fixed all issues related to refetchQueries. Only active queries are refetched, even if DocumentNode references aren't matching, inactive queries are still showing up as a warning. 🎉

It means we will be able to finalize the migration from the legacy { query, variables } or even query names as magic strings to fully embrace reusing DocumentNodes, super exciting!

jerelmiller

This change looks great!

The only change I'm requesting here is to have a test case for passing an anonymous query document to refetchQueries so that we can at least have that error message written down somewhere.

Let me know what you think about having a fallback name so that we can avoid the hideous error message. Even if it goes unchanged, at least its an improvement from before since it prints the query string instead of the AST.

.changeset/gorgeous-sheep-knock.md

src/core/__tests__/QueryManager/index.ts

jerelmiller · 2024-12-18T22:36:44Z

src/core/__tests__/QueryManager/index.ts

+      );
+      expect(observable.getCurrentResult().data).toEqual(secondReqData);
+
+      await wait(10);


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.

src/core/__tests__/QueryManager/index.ts

jerelmiller · 2024-12-18T22:49:58Z

src/core/QueryManager.ts

-            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`,


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!

Co-authored-by: Jerel Miller <[email protected]>

charpeni · 2024-12-19T15:41:05Z

src/core/QueryManager.ts

+          const isQueryString =
+            nameOrQueryString.startsWith("query ") ||
+            nameOrQueryString.startsWith("{"); // Shorthand anonymous queries


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.

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?

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!

src/core/QueryManager.ts

Co-authored-by: Jerel Miller <[email protected]>

jerelmiller · 2024-12-19T16:20:45Z

by the way, ignore that failing test case with expect(onCompleted).toHaveBeenCalledTimes(1);. We have a flaky test that we keep forgetting to fix. That failure is not caused by this PR.

jerelmiller

💯 Thanks so much for fixing this! I'll get this in the next patch release 🙂

Add test for refetchQueries with a DocumentNode

d04679f

Test different references of a same query DocumentNode

19d14a7

charpeni added 3 commits December 18, 2024 14:58

Compare DocumentNode as strings via print

9f4b665

Add changeset

521b9d1

Run Prettier

71f4652

charpeni commented Dec 18, 2024

View reviewed changes

charpeni marked this pull request as ready for review December 18, 2024 20:47

jerelmiller reviewed Dec 18, 2024

View reviewed changes

charpeni and others added 5 commits December 18, 2024 18:27

Set changeset as a patch

ac07d25

Co-authored-by: Jerel Miller <[email protected]>

Simplify changeset

75c3420

Co-authored-by: Jerel Miller <[email protected]>

Remove legacyOneTimeQuery reference

3c10ea5

Co-authored-by: Jerel Miller <[email protected]>

Remove legacyOneTimeQuery reference

7873f3c

Co-authored-by: Jerel Miller <[email protected]>

Only show query name and support anonymous queries

8860968

charpeni commented Dec 19, 2024

View reviewed changes

Update .api-reports

1dfb266

jerelmiller reviewed Dec 19, 2024

View reviewed changes

src/core/QueryManager.ts Outdated Show resolved Hide resolved

Fix grammar for anonymous query

b632d5d

Co-authored-by: Jerel Miller <[email protected]>

charpeni and others added 4 commits December 19, 2024 11:21

Fix anonymous query test

af15100

Simplify the logic to get the query name

17c8056

Update .api-reports

95cd778

Update size limits

03334ab

jerelmiller approved these changes Dec 19, 2024

View reviewed changes

jerelmiller merged commit 4334d30 into apollographql:main Dec 19, 2024
40 of 43 checks passed

github-actions bot mentioned this pull request Dec 19, 2024

Version Packages #12237

Merged

charpeni deleted the 12164-refetch-queries-with-document-node branch December 19, 2024 16:45

charpeni mentioned this pull request Dec 19, 2024

refetchQueries not working when using string array after mutation #5419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare `DocumentNode` used in `refetchQueries` as strings #12236

Compare `DocumentNode` used in `refetchQueries` as strings #12236

charpeni commented Dec 18, 2024 •

edited

Loading

netlify bot commented Dec 18, 2024 •

edited

Loading

svc-apollo-docs commented Dec 18, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 18, 2024 •

edited

Loading

changeset-bot bot commented Dec 18, 2024 •

edited

Loading

jerelmiller commented Dec 18, 2024

charpeni Dec 18, 2024

jerelmiller Dec 18, 2024 •

edited

Loading

charpeni Dec 18, 2024

jerelmiller Dec 19, 2024

charpeni Dec 19, 2024

charpeni commented Dec 18, 2024

jerelmiller left a comment

jerelmiller Dec 18, 2024

jerelmiller Dec 18, 2024 •

edited

Loading

charpeni Dec 19, 2024

jerelmiller Dec 19, 2024

charpeni Dec 19, 2024

jerelmiller commented Dec 19, 2024 •

edited

Loading

jerelmiller left a comment

Compare DocumentNode used in refetchQueries as strings #12236

Compare DocumentNode used in refetchQueries as strings #12236

Conversation

charpeni commented Dec 18, 2024 • edited Loading

netlify bot commented Dec 18, 2024 • edited Loading

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

svc-apollo-docs commented Dec 18, 2024 • edited Loading

✅ Docs Preview Ready

pkg-pr-new bot commented Dec 18, 2024 • edited Loading

changeset-bot bot commented Dec 18, 2024 • edited Loading

🦋 Changeset detected

jerelmiller commented Dec 18, 2024

Choose a reason for hiding this comment

jerelmiller Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charpeni commented Dec 18, 2024

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller commented Dec 19, 2024 • edited Loading

jerelmiller left a comment

Choose a reason for hiding this comment

Compare `DocumentNode` used in `refetchQueries` as strings #12236

Compare `DocumentNode` used in `refetchQueries` as strings #12236

charpeni commented Dec 18, 2024 •

edited

Loading

netlify bot commented Dec 18, 2024 •

edited

Loading

svc-apollo-docs commented Dec 18, 2024 •

edited

Loading

pkg-pr-new bot commented Dec 18, 2024 •

edited

Loading

changeset-bot bot commented Dec 18, 2024 •

edited

Loading

jerelmiller Dec 18, 2024 •

edited

Loading

jerelmiller Dec 18, 2024 •

edited

Loading

jerelmiller commented Dec 19, 2024 •

edited

Loading