`useQuery`: delay unsubscribe to fix race conditions #10629

phryneas · 2023-03-07T13:32:04Z

This changes the behavior found in #10540 and fixes #9819.

Checklist:

If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests

Discussion

This change significantly changes how disableNetworkFetches behaves in some situations:

Assuming a scenario where disableNetworkFetches is true, a component using useQuery renders, disableNetworkFetches changes to false and the component using useQuery rerenders, we have this behavior change:

The good

Where before, cache-and-network, network-only, and no-cache ran their queries twice on an empty cache, those now only run on the first render.
This is generally desirable behavior.

The same happens for no-cache when a cache entry is present: two requests are down to one.

The questionable:

Before, cache-and-network and network-only did not run their queries when a cache entry was found but ran a query once disableNetworkFetches was turned on (and the component rerendered for any reason).
Now they don't run that network request.

That behavior was flimsy (it only occurred when the component was rerendered for any reason), but I can think of good and bad arguments for why it should be the expected behavior - our docs don't help me there.

So the question is: are we okay with this change?
If yes: Great, let's merge this and call it a day.
But if we would want network requests to be triggered once disableNetworkFetches flips from true to false, we might revisit something along the lines of #8721

changeset-bot · 2023-03-07T13:32:08Z

🦋 Changeset detected

Latest commit: 60dd769

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

phryneas · 2023-03-07T13:34:01Z

src/react/hooks/__tests__/useQuery.test.tsx

@@ -1746,26 +1755,37 @@ describe('useQuery Hook', () => {
      expect(result.current.data).toBe(undefined);

      await waitFor(() => {
-        expect(result.current.loading).toBe(false);


This test tested if a single query was made but never tested if there was any actual polling going on - that's why we never were aware of this bug.

src/react/hooks/__tests__/useQuery.test.tsx

benjamn

Thanks for fixing this and adding tests @phryneas!

benjamn · 2023-03-07T20:55:50Z

src/react/hooks/__tests__/useQuery.test.tsx

+    test.each<
+      [
+        fetchPolicy: WatchQueryFetchPolicy,
+        initialQueryValue: TestQueryValue | undefined,
+        shouldFetchOnFirstRender: boolean,
+        shouldFetchOnSecondRender: boolean
+      ]
+    >([
+      [`cache-first`, emptyData, true, false],
+      [`cache-first`, cacheData, false, false],
+      [`cache-only`, emptyData, false, false],
+      [`cache-only`, cacheData, false, false],
+      [`cache-and-network`, emptyData, true, false],
+      [`cache-and-network`, cacheData, false, false],
+      [`network-only`, emptyData, true, false],
+      [`network-only`, cacheData, false, false],
+      [`no-cache`, emptyData, true, false],
+      [`no-cache`, cacheData, true, false],
+      [`standby`, emptyData, false, false],
+      [`standby`, cacheData, false, false],
+    ])(
+      "fetchPolicy %s, cache: %p should fetch during `disableNetworkFetches`: %p and after `disableNetworkFetches` has been disabled: %p",
+      async (policy, initialQueryValue, shouldFetchOnFirstRender, shouldFetchOnSecondRender) => {


I love this style of writing lists of test cases! Please feel free to refactor any other places where you see something similar but uglier, if you come across any.

benjamn · 2023-03-07T21:05:38Z

src/react/hooks/useQuery.ts

+        // Do the "unsubscribe" with a short delay.
+        // This way, an existing subscription can be reused without an additional
+        // request if "unsubscribe"  and "resubscribe" to the same ObservableQuery 
+        // happen in very fast succession.
+        return () => setTimeout(() => subscription.unsubscribe());


Some precedent for this technique:

apollo-client/src/utilities/observables/Concast.ts

Line 200 in 355dce8

if (sub) setTimeout(() => sub.unsubscribe());

If the delay between the last unsubscription and the resubscription ever gets longer than the minimum setTimeout delay, that seems like it could reintroduce this problem unpredictably, but the consequences would be the same as without this change.

Yup, this is mostly good if the unsubscribe and subscribe are called synchronously - for everything else, it's just a band-aid - but at some level of delay, a refetch might actually be intended.

trevor-scheer · 2023-03-08T14:16:39Z

Thanks for fixing this one @phryneas!

commit 72b222e Merge: def116b afccbdb Author: Gaston Morixe <gaston@gastonmorixe.com> Date: Fri Mar 10 18:23:57 2023 -0300 Merge branch 'main' into gm/fix/merge-function-called-before-deepmerge-sep202022 commit afccbdb Author: Trevin Avery <TrevinAvery@gmail.com> Date: Wed Mar 8 11:18:47 2023 -0800 Fix snapshot with non-ascii character (apollographql#10611) commit 02605bb Author: Lenz Weber-Tronic <lorenz.weber-tronic@apollographql.com> Date: Wed Mar 8 10:18:29 2023 +0100 `useQuery`: delay unsubscribe to fix race conditions (apollographql#10629) * add test for current behavior * add failing test for polling * slightly delay unsubscribe * adjust tests to new behavior commit 355dce8 Author: Jeff Auriemma <bignimbus@users.noreply.github.com> Date: Tue Mar 7 11:47:45 2023 -0500 Bump date in roadmap commit 3b504f7 Author: Jeff Auriemma <bignimbus@users.noreply.github.com> Date: Tue Mar 7 11:41:42 2023 -0500 Update dates and notes for upcoming releases commit c4daaf6 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Mar 6 15:34:20 2023 -0500 chore(deps): update dependency graphql-ws to v5.12.0 (apollographql#10625) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Alessia Bellisario <alessia@apollographql.com> commit 88993a2 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Mar 6 14:02:53 2023 -0500 chore(deps): update dependency @types/node to v18.14.6 (apollographql#10622) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit aa81522 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Mon Mar 6 14:01:45 2023 -0500 chore(deps): update dependency rimraf to v4.3.0 (apollographql#10623) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit e4fb0ca Author: Henry Q. Dineen <henryqdineen@users.noreply.github.com> Date: Fri Mar 3 12:14:58 2023 -0500 remove obsolete @types/fast-json-stable-stringify dev dep (apollographql#10617) commit 260914a Author: Trevin Avery <TrevinAvery@gmail.com> Date: Fri Mar 3 08:43:01 2023 -0800 Update batchLink tests to use fake timers (apollographql#10612) * Update batchLink tests to use fake timers * fix: refactor batch interval test to support fake timers --------- Co-authored-by: Alessia Bellisario <alessia@apollographql.com> commit ebee40b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Fri Mar 3 10:49:59 2023 -0500 chore(deps): update dependency eslint to v8.35.0 (apollographql#10613) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 5161f09 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 17:41:57 2023 -0700 chore(deps): update dependency @types/node to v18.14.4 (apollographql#10602) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit 7823843 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 17:31:57 2023 -0700 chore(deps): update cimg/node docker tag to v19.7.0 (apollographql#10605) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit f8fed6b Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 16:59:56 2023 -0700 chore(deps): update dependency @babel/parser to v7.21.2 (apollographql#10606) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit e9f0dd5 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 16:45:50 2023 -0700 chore(deps): update dependency @types/glob to v8.1.0 (apollographql#10607) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit bbb04b0 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 16:32:10 2023 -0700 chore(deps): update dependency terser to v5.16.5 (apollographql#10603) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit f9868b8 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 16:03:25 2023 -0700 chore(deps): update dependency @typescript-eslint/eslint-plugin to v5.54.0 (apollographql#10608) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit ef13979 Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 15:43:19 2023 -0700 chore(deps): update jaywcjlove/github-action-package action to v1.3.1 (apollographql#10604) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit e6d552f Author: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Date: Thu Mar 2 15:33:26 2023 -0700 chore(deps): update dependency @typescript-eslint/parser to v5.54.0 (apollographql#10609) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> commit def116b Merge: a32e0d2 6bd651b Author: Gaston Morixe <gaston@gastonmorixe.com> Date: Tue Sep 20 11:57:40 2022 -0500 Merge tag 'v3.6.8' into gm/fix/merge-function-called-before-deepmerge-sep202022 commit a32e0d2 Author: Gaston Morixe <gaston@gastonmorixe.com> Date: Tue Mar 8 17:38:31 2022 -0300 custom version commit 42a20f8 Author: Gaston Morixe <gaston@gastonmorixe.com> Date: Tue Mar 8 17:07:12 2022 -0300 increatese maxSize to 28.65 commit 045a9d5 Author: Gaston Morixe <gaston@gastonmorixe.com> Date: Mon Mar 7 18:35:02 2022 -0300 Run Policies's merge before cache merge adds test fix formatting

phryneas · 2023-03-29T11:31:19Z

/release:pr

phryneas · 2023-03-29T12:05:31Z

/release:pr

github-actions · 2023-03-29T12:07:06Z

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-10629-20230329120619.

Lenz Weber-Tronic added 5 commits March 7, 2023 14:12

add test for current behavior

bdd733f

add failing test for polling

da9f2c4

slightly delay unsubscribe

be5b90b

adjust tests to new behaviour

c68afa1

changeset

6669420

phryneas requested review from benjamn, alessbell and jerelmiller March 7, 2023 13:32

phryneas commented Mar 7, 2023

View reviewed changes

src/react/hooks/__tests__/useQuery.test.tsx Outdated Show resolved Hide resolved

Update src/react/hooks/__tests__/useQuery.test.tsx

cd8ed84

benjamn approved these changes Mar 7, 2023

View reviewed changes

Merge branch 'main' into pr/10540

60dd769

phryneas merged commit 02605bb into main Mar 8, 2023

phryneas deleted the pr/10540 branch March 8, 2023 09:18

github-actions bot mentioned this pull request Mar 8, 2023

Version Packages #10630

Merged

phryneas mentioned this pull request Mar 8, 2023

useQuery pollInterval stop working in React v18 StrictMode #9819

Closed

github-actions bot mentioned this pull request Mar 15, 2023

Version Packages (alpha) #10636

Merged

phryneas mentioned this pull request Mar 29, 2023

Queries cancelled in strict mode with nextFetchPolicy: "cache-first" #9989

Closed

phryneas restored the pr/10540 branch March 29, 2023 12:05

jerelmiller deleted the pr/10540 branch March 31, 2023 05:53

phryneas mentioned this pull request Apr 13, 2023

useSubscription: keep subscription Concast alive if new in-flight subscription subscribes at the same time last in-flight subscription unsubscribes #10718

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`useQuery`: delay unsubscribe to fix race conditions #10629

`useQuery`: delay unsubscribe to fix race conditions #10629

phryneas commented Mar 7, 2023

changeset-bot bot commented Mar 7, 2023 •

edited

Loading

phryneas Mar 7, 2023

benjamn left a comment

benjamn Mar 7, 2023

benjamn Mar 7, 2023

phryneas Mar 7, 2023

trevor-scheer commented Mar 8, 2023

phryneas commented Mar 29, 2023

phryneas commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

useQuery: delay unsubscribe to fix race conditions #10629

useQuery: delay unsubscribe to fix race conditions #10629

Conversation

phryneas commented Mar 7, 2023

Checklist:

Discussion

The good

The questionable:

changeset-bot bot commented Mar 7, 2023 • edited Loading

🦋 Changeset detected

phryneas Mar 7, 2023

Choose a reason for hiding this comment

benjamn left a comment

Choose a reason for hiding this comment

benjamn Mar 7, 2023

Choose a reason for hiding this comment

benjamn Mar 7, 2023

Choose a reason for hiding this comment

phryneas Mar 7, 2023

Choose a reason for hiding this comment

trevor-scheer commented Mar 8, 2023

phryneas commented Mar 29, 2023

phryneas commented Mar 29, 2023

github-actions bot commented Mar 29, 2023

`useQuery`: delay unsubscribe to fix race conditions #10629

`useQuery`: delay unsubscribe to fix race conditions #10629

changeset-bot bot commented Mar 7, 2023 •

edited

Loading