Fixed useQuery under StrictMode creating 2 observable query #11925

Cellule · 2024-07-04T13:37:05Z

Closes #11914

netlify · 2024-07-04T13:37:10Z

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

Name	Link
🔨 Latest commit	`e568c78`

changeset-bot · 2024-07-04T13:37:10Z

🦋 Changeset detected

Latest commit: e568c78

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

I believe the fix you presented here is a reasonable one and I'm good with that fix!

Where I'd like to see a change potentially is how we setup the test to reproduce this behavior. See my comments on the tests and let me know what you think!

jerelmiller · 2024-07-08T22:50:47Z

src/core/QueryInfo.ts

 import { equal } from "@wry/equality";
+import type { DocumentNode, GraphQLError } from "graphql";


It looks like your editor might have applied some kind of import reordering. Would you mind disabling that and reverting the changes to the imports? We'd love to keep the git blame as clean as possible 🙂

jerelmiller · 2024-07-08T22:51:12Z

src/core/__tests__/QueryManager/index.ts

@@ -1,53 +1,53 @@
 // externals
+import { DocumentNode, GraphQLError } from "graphql";


Same here. Would you mind reverting the change that reordered these imports?

jerelmiller · 2024-07-08T22:56:40Z

src/core/__tests__/QueryManager/index.ts

+        });
+        // Recreate state where an observable query is dirty but has no observers in the query manager
+        // @ts-expect-error -- Accessing private field for testing
+        oq.queryInfo.dirty = true;


While this no doubt reproduces the issues, I'd still love to figure out how best the client gets into this state to begin with using user-facing APIs. User's aren't going to be touching QueryInfo or the dirty flag manually, so the more we can understand the situation that gets us here, the better. It will help us avoid a regression in the future knowing exactly the situation that got us here.

Since we are able to reproduce in CodeSandbox using useQuery with React's strict mode enabled, we should consider moving this test over to the useQuery tests. Maybe we start there to figure out exactly how this dirty flag gets set and work backwards?

Once we have a failing test for useQuery ("failing" means assuming the change you've added to QueryInfo is not applied), then we might figure out how best to backport the situation over to this test.

What do you think about that?

My idea was to reproduce the bad state, regardless of what causes the bad state
I agree that it's better to do a fully functional test especially to identify the "real" cause of the issue, but I haven't had time to look into it this week because I was swamped and I'm going on vacations starting next week.

If you want to pick up the PR to wrap it up I don't mind.
Otherwise I'll try to look into it when I get a chance (it's still on my list 😅)

@jerelmiller it was great to chat with you yesterday at the GraphQL Summit!
I rebased my fix and gave a shot at writing a test with useQuery.
I have not been able to reproduce the issue in the test however :(
Happy to get your input on my test how you think we can bring this to the finish line

svc-apollo-docs · 2024-10-09T14:59:22Z

✅ Docs Preview Ready

No new or changed pages found.

…resetting the store Closes apollographql#11914

Cellule · 2024-10-11T16:36:49Z

src/core/QueryManager.ts

@@ -880,10 +880,7 @@ export class QueryManager<TStore> {
          options: { fetchPolicy },
        } = oq;

-        if (
-          fetchPolicy === "standby" ||
-          (include === "active" && !oq.hasObservers())


This part is related to this part of the issue my colleague found

#11914 (comment)

This code is 3 years old, so I'm skeptical that my change makes sense.
From my perspective, if someone provided a list of DocumentNode or OperationName to refetch, they clearly meant to refetch only active ones.
I can't think of a reason why you'd want to refetch queries without any observers

Humm looks like I was wrong

apollo-client/src/__tests__/refetchQueries.ts

Lines 535 to 549 in 8047181

itAsync(

"includes queries named in refetchQueries even if they have no observers",

async (resolve, reject) => {

const client = makeClient();

const aObs = client.watchQuery({ query: aQuery });

const bObs = client.watchQuery({ query: bQuery });

const abObs = client.watchQuery({ query: abQuery });

// These ObservableQuery objects have no observers yet, but should

// nevertheless be refetched if identified explicitly in an options.include

// array passed to client.refetchQueries.

expect(aObs.hasObservers()).toBe(false);

expect(bObs.hasObservers()).toBe(false);

expect(abObs.hasObservers()).toBe(false);

I still don't know why you'd want that, but there must be a good reason.
This comes back to why we end up with queries without observers with useQuery

Huh, I didn't realize that was a thing. Let me ask around and see if I can figure out why we'd ever want this. I agree that fetching a query with no observers is an odd behavior 🤔

I'm noodling on the rest of the implementation, but I'm curious... with those other changes, do we need to modify this? If an ObservableQuery is created but not registered, it shouldn't be included in this.queries correct?

I'm wondering if we should leave this alone for now. To be honest, I'm not sure how much of a breaking change this is and while I think it makes sense to you and I that this should be the correct behavior, we've had fiery discussions on far smaller changes 😂. We will be starting work on v4 here very soon, so I think we could probably punt this change to there where we can announce this change more formally.

Fair, I don't mind living with this part as a patch on our side and maybe we can surface up reasons to not do it XD
If we manage to completely solve the StrictMode issue so that QueryManager never has "dead" queries then this change is not as important anymore either

Yep exactly!

…d of a useState initializer

Cellule · 2024-10-17T20:27:27Z

@jerelmiller Let me know how you feel about the direction I've taken in my PR to fix the StrictMode issue before I dig in more into the current test failures

jerelmiller

Took me a bit to come around to this solution, but I kind of dig it. I'd like to discuss this with the team in our next team meeting as well in case someone else has a suggestion that I'm not thinking of, but otherwise I think this is a positive change.

For the changes there now, I do have a few suggestions that I think could simplify a touch more without the need to pass around the queryInfo or register function. Let me know what you think!

jerelmiller · 2024-10-18T16:53:39Z

src/core/QueryManager.ts

+  /**
+   * Create a query, but do not associate it with the QueryManager.
+   * This allows to throw away the query if it ends up not being needed
+   */


Totally fine if you want to keep this comment, but just an FYI we consider QueryManager a private class so it will only ever be used internally. I'm not opposed to more documentation, but this won't be seen by consumers of this library 🙂

jerelmiller · 2024-10-18T16:55:53Z

src/core/QueryManager.ts

+  public registerObservableQuery(
+    observable: ObservableQuery<any, any>,
+    queryInfo: QueryInfo
+  ): void {
+    this.queries.set(observable.queryId, queryInfo);
+  }


Suggested change

public registerObservableQuery(

observable: ObservableQuery<any, any>,

queryInfo: QueryInfo

): void {

this.queries.set(observable.queryId, queryInfo);

}

public registerObservableQuery(

observable: ObservableQuery<any, any>

): void {

this.queries.set(observable.queryId, observable['queryInfo']);

}

When we instantiate ObservableQuery, it gets queryInfo in its constructor and sets it as a private property. I'd say we can shorten this to just take the observable and access that queryInfo from there using the bracket notation which lets us get access to private properties.

jerelmiller · 2024-10-18T16:57:48Z

src/core/QueryManager.ts

@@ -756,6 +760,25 @@ export class QueryManager<TStore> {
      variables: observable.variables,
    });

+    return { observable, queryInfo };


With the change suggested for registerObservableQuery, we shouldn't need to return queryInfo here since its set as a property on observable. I'd shorten this to just return the observable directly which should simplify some things.

Suggested change

return { observable, queryInfo };

return observable;

jerelmiller · 2024-10-18T17:05:25Z

src/core/QueryInfo.ts

+      !this.listeners.size ||
+      // It's possible that the query is no longer being watched, but the
+      // ObservableQuery is still active/pending cleanup. In this case, we should not notify.
+      !this.observableQuery?.hasObservers()


I think we probably want to call this change out in the changelog in case we have any users that use the core watchQuery API directly. I'd say something along the lines of:

Cache writes to ObservableQuery with no observers will no longer receive cache updates or trigger refetches as a result of cache changes. This was usually caused when using useQuery with React's strict mode which could cause the creation of multiple ObservableQuery instances on mount. This change should only affect you if you create ObservableQuery instances via watchQuery and do not subscribe to them but expect cache updates and refetches to occur.

Obviously feel free to workshop this and make it your own, really as long as we have something here is what I care about.

Feel free to add this as a separate changeset since I'd view it as a separate change from the strict mode issue.

After talking with the team, I think we might want to move this change to 4.0 and make it a breaking change so that we can call it out prominently. Its difficult to tell who might rely on this behavior if you're using the core API directly. I think the fix for strict mode might be enough here to address what you're seeing specifically in your app. Moving this to a major allows us to ensure we add this behavior everywhere as well.

As an FYI, we will begin work on 4.0 once 3.12 ships (a few weeks from now), so shouldn't be long before we can address this 🙂

jerelmiller · 2024-10-18T17:08:29Z

src/core/ApolloClient.ts

+    return observable;
+  }
+
+  public createQuery<


I'd make this a private method. We treat public methods as part of the public API which means we can't make breaking changes to this after this is released and I'd prefer we keep this out of users hands if we can. By making it private, that should allow us to tweak it/rename it/etc. without that fear.

Since you use this from useQuery, TypeScript will obviously complain when you make this change, so to get around that, use bracket notation:

// useQuery.ts client['createQuery'](options);

jerelmiller · 2024-10-18T17:13:00Z

src/core/ApolloClient.ts

+    return {
+      observable,
+      register: () => {
+        this.queryManager.registerObservableQuery(observable, queryInfo);
+      },
+    };


With the suggested change to registeerObservableQuery, I'd actually suggest that we don't need to return this register function here and can just return the observable. We can then use queryManager.reigsterObservableQuery directly in place of this instead.

Suggested change

return {

observable,

register: () => {

this.queryManager.registerObservableQuery(observable, queryInfo);

},

};

return observable;

jerelmiller · 2024-10-18T19:44:12Z

src/core/__tests__/QueryManager/index.ts

+        });
+        // Recreate state where an observable query is dirty but has no observers in the query manager
+        // @ts-expect-error -- Accessing private field for testing
+        oq.queryInfo.dirty = true;


I'm curious, now that we've sorta pinned this down on both strict mode and the creation of multiple ObservableQuery instances without observers, do we have a need for this condition anymore?

jerelmiller · 2024-10-22T15:57:11Z

I asked the team about this and my colleague pointed out something that we do in useSubscription that we may or may not be able to do here. Check out this bit of code:

apollo-client/src/react/hooks/useSubscription.ts

Lines 354 to 363 in 6496b25

    
           return Object.assign( 
        
             new Observable<FetchResult<TData>>((observer) => { 
        
               // lazily start the subscription when the first observer subscribes 
        
               // to get around strict mode 
        
               if (!observable) { 
        
                 observable = client.subscribe(options); 
        
               } 
        
               const sub = observable.subscribe(observer); 
        
               return () => sub.unsubscribe(); 
        
             }),

Here we wrap the subscription with its own Observable so that we only create the ObservableQuery when its subscribed. Not sure if you want to perhaps noodle on this? This might bring its own challenges with useQuery, but if you're able to get something working like this then we can isolate the changes to useQuery itself without having to touch the core API.

Thoughts?

Cellule · 2024-10-23T13:24:35Z

I asked the team about this and my colleague pointed out something that we do in useSubscription that we may or may not be able to do here. Check out this bit of code:

apollo-client/src/react/hooks/useSubscription.ts

Lines 354 to 363 in 6496b25

return Object.assign(

new Observable<FetchResult<TData>>((observer) => {

// lazily start the subscription when the first observer subscribes

// to get around strict mode

if (!observable) {

observable = client.subscribe(options);

}

const sub = observable.subscribe(observer);

return () => sub.unsubscribe();

}),

Here we wrap the subscription with its own Observable so that we only create the ObservableQuery when its subscribed. Not sure if you want to perhaps noodle on this? This might bring its own challenges with useQuery, but if you're able to get something working like this then we can isolate the changes to useQuery itself without having to touch the core API.

Thoughts?

So I was digging into this, I haven't able to easily replicate that kind of behavior for useQuery.
My biggest issue was that too much code relies on the ObservableQuery to be readily available and returned to the user through obsQueryFields
It would likely be possible to make it work, but as I started hacking at it, it got messy fast.

So instead I considered another angle, since we want to register the query in the QueryManager when the ObservableQuery subscribes, why not let it take care of subscribing.
The ObservableQuery is also in charge of removing itself through tearDownQuery so I thought it made sense.
I removed this.queries.set(observable.queryId, queryInfo); from QueryManager.watchQuery and added this.queryManager["queries"].set(this.queryId, this.queryInfo); in the subscribe function of ObservableQuery.
It worked in my scenario, but I noticed that a few other tests failed, namely in createQueryPreloader and some others. The tests looked fine, so I was suspicious.
I'm not sure why you would want to register a query that you won't subscribe to, but anyway.

So now I'm looking at a solution to have this behavior only in useQuery.
I passed a hidden option through WatchQueryOption that would only set the query in QueryManager.queries in the ObservableQuery subscribe function.
It seems to work, but look a bit hackish (maybe less than my current solution?)

Boy oh boy, I thought the solution worked because I only tested on React19, but it seems on React18 and 17 the behavior of useSyncExternalStore causes to call the getSnapshot function breaking React rules which registers the query in the ApolloCache
When another query gets result, it would broadcast to all cache watches causing the query to reobserve.
I initially hid/mitigated this part with the shouldNotify "fix"

Now I really feel like we might need to go down the path of only creating the ObservableQuery when it actually subscribes 😮‍💨

jerelmiller · 2024-11-08T23:45:02Z

Gah. I had another conference to attend then was on vacation. Apologies for my slow time to respond!

I will take a look at your changes and respond to your latest comment next week. Thanks so much for your patience!

Cellule · 2024-11-09T00:49:34Z

Gah. I had another conference to attend then was on vacation. Apologies for my slow time to respond!

I will take a look at your changes and respond to your latest comment next week. Thanks so much for your patience!

Haha no worries, glad it's still on your radar. The current state of my PR is not really great hahaha.
I feel I might have to pull my sleeve and do the dirty work. Let me know what you think from my other comments then I can try to take another go at it 😁

jerelmiller self-requested a review July 5, 2024 21:38

jerelmiller requested changes Jul 8, 2024

View reviewed changes

Cellule force-pushed the 11914-resetStore branch from 187828a to 98157d9 Compare October 9, 2024 14:59

Fixed issue causing inactive queries to be executed when clearing or …

823b26e

…resetting the store Closes apollographql#11914

Cellule force-pushed the 11914-resetStore branch from 98157d9 to 823b26e Compare October 9, 2024 15:45

Do not refetch dead queries when referenced by name

0db43d3

Cellule commented Oct 11, 2024

View reviewed changes

Cellule added 5 commits October 11, 2024 12:38

Remove leftout .only debugging

1e23e8e

Register Observable Query with the QueryManager in a useEffect instea…

4c71b46

…d of a useState initializer

Fix test and formatting

7edb03f

Fix observable registration in SSR mode

28dd4e3

Merge remote-tracking branch 'origin/main' into 11914-resetStore

8a7a539

Cellule changed the title ~~Fixed issue causing inactive queries to be executed when clearing or resetting the store~~ Fixed useQuery under StrictMode creating 2 observable query Oct 17, 2024

Cellule added 2 commits October 18, 2024 12:29

Revert QueryManager.getObservableQueries change

1500206

Update tests after revert

e568c78

jerelmiller requested changes Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed useQuery under StrictMode creating 2 observable query #11925

Fixed useQuery under StrictMode creating 2 observable query #11925

Cellule commented Jul 4, 2024

netlify bot commented Jul 4, 2024 •

edited

Loading

changeset-bot bot commented Jul 4, 2024 •

edited

Loading

jerelmiller left a comment

jerelmiller Jul 8, 2024

jerelmiller Jul 8, 2024

jerelmiller Jul 8, 2024 •

edited

Loading

Cellule Jul 19, 2024

Cellule Oct 9, 2024

svc-apollo-docs commented Oct 9, 2024 •

edited

Loading

Cellule Oct 11, 2024

Cellule Oct 14, 2024 •

edited

Loading

jerelmiller Oct 15, 2024

jerelmiller Oct 18, 2024

Cellule Oct 18, 2024

jerelmiller Oct 18, 2024

Cellule commented Oct 17, 2024

jerelmiller left a comment

jerelmiller Oct 18, 2024

jerelmiller Oct 18, 2024

jerelmiller Oct 18, 2024

jerelmiller Oct 18, 2024

jerelmiller Oct 22, 2024

jerelmiller Oct 18, 2024

jerelmiller Oct 18, 2024

jerelmiller Oct 18, 2024

jerelmiller commented Oct 22, 2024

Cellule commented Oct 23, 2024

jerelmiller commented Nov 8, 2024

Cellule commented Nov 9, 2024

		import { equal } from "@wry/equality";
		import type { DocumentNode, GraphQLError } from "graphql";

		@@ -1,53 +1,53 @@
		// externals
		import { DocumentNode, GraphQLError } from "graphql";

	itAsync(
	"includes queries named in refetchQueries even if they have no observers",
	async (resolve, reject) => {
	const client = makeClient();

	const aObs = client.watchQuery({ query: aQuery });
	const bObs = client.watchQuery({ query: bQuery });
	const abObs = client.watchQuery({ query: abQuery });

	// These ObservableQuery objects have no observers yet, but should
	// nevertheless be refetched if identified explicitly in an options.include
	// array passed to client.refetchQueries.
	expect(aObs.hasObservers()).toBe(false);
	expect(bObs.hasObservers()).toBe(false);
	expect(abObs.hasObservers()).toBe(false);

Fixed useQuery under StrictMode creating 2 observable query #11925

Are you sure you want to change the base?

Fixed useQuery under StrictMode creating 2 observable query #11925

Conversation

Cellule commented Jul 4, 2024

netlify bot commented Jul 4, 2024 • edited Loading

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

changeset-bot bot commented Jul 4, 2024 • edited Loading

🦋 Changeset detected

jerelmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerelmiller Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-apollo-docs commented Oct 9, 2024 • edited Loading

✅ Docs Preview Ready

Choose a reason for hiding this comment

Cellule Oct 14, 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

Choose a reason for hiding this comment

Cellule commented Oct 17, 2024

jerelmiller left a comment

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

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

Choose a reason for hiding this comment

jerelmiller commented Oct 22, 2024

Cellule commented Oct 23, 2024

jerelmiller commented Nov 8, 2024

Cellule commented Nov 9, 2024

netlify bot commented Jul 4, 2024 •

edited

Loading

changeset-bot bot commented Jul 4, 2024 •

edited

Loading

jerelmiller Jul 8, 2024 •

edited

Loading

svc-apollo-docs commented Oct 9, 2024 •

edited

Loading

Cellule Oct 14, 2024 •

edited

Loading