`useBackgroundQuery`: remove `promiseCache`, work around race condition #11366

phryneas · 2023-11-16T11:21:05Z

This one moves quite a lot of parts around - let's best have a call at some point to talk about it @jerelmiller :)

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

changeset-bot · 2023-11-16T11:21:08Z

⚠️ No Changeset found

Latest commit: 4bde5a0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

github-actions · 2023-11-16T11:24:29Z

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.71 KB (-0.04% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	44.22 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.72 KB (+0.03% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.87 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.56 KB (-0.01% 🔽)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.21 KB (0%)
import { useQuery } from "dist/react/index.js"	4.33 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.15 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	4.63 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.46 KB (+0.03% 🔺)
import { useMutation } from "dist/react/index.js"	2.6 KB (+0.04% 🔺)
import { useMutation } from "dist/react/index.js" (production)	2.58 KB (+0.08% 🔺)
import { useSubscription } from "dist/react/index.js"	2.29 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.24 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	4.29 KB (+0.12% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production)	3.7 KB (+0.11% 🔺)
import { useBackgroundQuery } from "dist/react/index.js"	3.88 KB (+2.14% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.28 KB (+2.41% 🔺)
import { useLoadableQuery } from "dist/react/index.js"	4.14 KB (+1.66% 🔺)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.55 KB (+2.2% 🔺)
import { useReadQuery } from "dist/react/index.js"	3.01 KB (+0.23% 🔺)
import { useReadQuery } from "dist/react/index.js" (production)	2.95 KB (-0.04% 🔽)
import { useFragment } from "dist/react/index.js"	2.1 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.05 KB (0%)

phryneas · 2023-11-16T11:32:12Z

src/react/cache/QueryReference.ts

-      this.reject = reject;
-    });
+    this.promise = withSequence(
+      wrapPromiseWithState(


We need to wrap them here, so secondIfNewerFulfilledOrFirst can know if the promise is already fulfilled.

That has a neat side effect:

Wrapping the promises with state here will save React a cycle of "start rendering" -> "suspend to wait for thenable because we don't know that it's already fulfilled" -> "resume rendering".

Async work becomes sync.

ya good call!

phryneas · 2023-11-16T11:34:26Z

src/react/hooks/useReadQuery.ts

  );
+  promise = secondIfNewerFulfilledOrFirst(promise, internalQueryRef.promise);


There are situations where the parent never re-rendered, and the above useSyncExternalStore listener was only active for the foreground render, not the background render.
In that case, the promise of this lane's QueryRef might still point towards an outdated promise. If the internalQueryRef.promise is fulfilled, and newer than promise, we want to priorize that one.

phryneas · 2023-11-16T11:42:12Z

src/utilities/promises/decoration.ts

 export type PromiseWithState<TValue> =
  | PendingPromise<TValue>
  | FulfilledPromise<TValue>
  | RejectedPromise<TValue>;

+export function withSequence<T extends Promise<any>>(


I'm not happy about the new export, maybe we could also just move the sequence stuff into the other helpers here?

Generally, there's probably a good opportunity to make things nicer here now :)

Meh not the end of the world. I think we should probably mark this as internal though:

/* @internal */

Thoughts?

we only care about identity, not about contents

…-useBackgroundQuery-promiseCache

jerelmiller

I'm mostly ok with the changes here, but the one that concerns me the most is the change in the suspense count for the test that is meant to re-suspend multiple times on refetch. Would you mind taking a look at that and providing an explanation as to why that change is necessary? Perhaps I'm missing something obvious.

jerelmiller · 2023-11-30T00:17:10Z

src/react/hooks/useBackgroundQuery.ts

+  if (unwrapQueryRef(wrapped.current)[0] !== queryRef) {
+    setWrappedQueryRef({ current: wrapQueryRef(queryRef, queryRef.promise) });
+  }
+  let wrappedQueryRef = wrapped.current;


Suggested change

let wrappedQueryRef = wrapped.current;

const wrappedQueryRef = wrapped.current;

Sorry 😅.

Better yet, since you're not reading this value anywhere but the return from this hook, perhaps you can just use wrapped.current there?

Also, the differentiation between wrapped and wrappedQueryRef shouldn't be necessary at all anymore - removing it :)

src/react/hooks/useBackgroundQuery.ts

jerelmiller · 2023-11-30T00:23:19Z

src/react/hooks/useBackgroundQuery.ts

+  const [wrapped, setWrappedQueryRef] = React.useState({
+    current: wrapQueryRef(queryRef, queryRef.promise),
+  });


I'm curious about the decision to create this as an object with a current property. It doesn't look like you do any mutable update to the current property and exclusively rely on setWrappedQueryRef.

Since wrapQueryRef will return a new object each time, that should be sufficient enough to ensure React won't bail on the render when comparing old and new values. Could you consider simplifying this to just set the wrapped query ref directly?

Suggested change

const [wrapped, setWrappedQueryRef] = React.useState({

current: wrapQueryRef(queryRef, queryRef.promise),

});

const [wrappedQueryRef, setWrappedQueryRef] = React.useState(() =>

wrapQueryRef(queryRef, queryRef.promise)

);

I think this might be a relic from the time before we had updateWrappedQueryRef and had to create a new one everytime. Removed it and all tests are still green.

jerelmiller · 2023-11-30T00:33:47Z

src/utilities/promises/decoration.ts


  // prevent potential edge cases leaking unhandled error rejections
  promise.catch(() => {});

  promise.status = "rejected";
  promise.reason = reason;

-  return promise;
+  return withSequence(promise);


I'd prefer to keep these methods free of the withSequence decoration and instead decorate promises that need sequences at the call sites that need them.

Suggested change

return withSequence(promise);

return promise;

jerelmiller · 2023-11-30T00:35:23Z

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

@@ -641,7 +641,7 @@ describe("useBackgroundQuery", () => {

    const [queryRef] = result.current;

-    const _result = await unwrapQueryRef(queryRef).promise;
+    const _result = await unwrapQueryRef(queryRef)[0].promise;


Oof I really gotta update these tests to use the new profiler helpers 😄. Would love to avoid testing against the query refs directly, but understand the change here!

jerelmiller · 2023-11-30T00:38:56Z

src/utilities/promises/decoration.ts

+  [sequence]: number;
+}
+
+export function secondIfNewerFulfilledOrFirst<TValue>(


It's still not clear to me if this does or doesn't solve the issue you mentioned on how we could show incorrect data if the child component rerenders in the "foreground" render while the background is suspended. Would you mind adding a test case for that in this PR so we can verify this does indeed work as we expect it to? I'd love to see that test fail when not using this helper, then pass as soon as its added back.

We have a test covering that - if you replace this function with just return first, the test

useBackgroundQuery › fetchMore › properly uses updateQuery when calling fetchMore

will fail.

Ha, I should have been more diligent about trying this. Awesome to hear!

jerelmiller · 2023-11-30T00:43:12Z

src/react/hooks/useReadQuery.ts

-  }
-
-  const promise = useSyncExternalStore(
+  let promise = useSyncExternalStore(


Suggested change

let promise = useSyncExternalStore(

const promise = useSyncExternalStore(

Perhaps I'm missing where its reassigned, but if not, could we use a const here?

It was before, but isn't anymore :) Good catch!

jerelmiller · 2023-11-30T00:45:12Z

src/react/hooks/useReadQuery.ts

@@ -36,32 +38,23 @@ export interface UseReadQueryResult<TData = unknown> {
 export function useReadQuery<TData>(
  queryRef: QueryReference<TData>
 ): UseReadQueryResult<TData> {
-  const internalQueryRef = unwrapQueryRef(queryRef);
-  invariant(


jerelmiller · 2023-11-30T00:48:10Z

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

-      // parent component re-suspends
-      expect(renders.suspenseCount).toBe(2);
+      // parent component didn't re-suspend
+      expect(renders.suspenseCount).toBe(1);


This doesn't seem quite right to me. Refetches should cause the component to re-suspend again unless the refetch is wrapped with a startTransition, so I would expect that clicking refetch twice after the initial load, would cause the suspense boundary to be shown 3 times.

jerelmiller · 2023-11-30T00:52:20Z

src/react/cache/QueryReference.ts

-      this.reject = reject;
-    });
+    this.promise = withSequence(
+      wrapPromiseWithState(


ya good call!

…romise

jerelmiller

I'll let you decide on the internal designation or not, but this looks great to me!

jerelmiller · 2023-11-30T19:09:22Z

src/react/cache/QueryReference.ts

+  return [
+    reference,
+    () =>
+      secondIfNewerFulfilledOrFirst(


I know I approved this already, but wanted to throw an alternate idea out here.

I think we can avoid the sequencing altogether by just checking to see if the promises are different from each other. If the reference.promise has changed, it means its been updated to a newer promise. In that way, you can then compare to see if the second has been fulfilled or not.

I just tried this change out with these tests and all continue to pass:

return [ reference, () => { const first = queryRef[PROMISE_SYMBOL]; const second = reference.promise; if (first === second) { return first; } return second.status === "fulfilled" ? second : first; }, ];

Returning first makes the tests fail like you described in my other comment.

How do you feel about this and removing the need for withSequence?

…n its fulfilled

jerelmiller

🎉 Nice work here! Great to see a potential bug caught before anyone else does 😆

…ackgroundQuery-promiseCache

to account for "empty" renders that don't actually render any components we're interested in testing

…ache

phryneas added 4 commits November 15, 2023 15:12

useSuspenseQuery: remove promiseCache

8ecd36f

missed cleanup

b8c51af

WIP status: useBackgroundQuery

79577b5

working reimplementation

1510019

phryneas force-pushed the pr/remove-useSuspenseQuery-promiseCache branch from 79577b5 to b8c51af Compare November 16, 2023 11:21

phryneas changed the title ~~useSuspenseQuery: remove promiseCache, work around race condition~~ useBackgroundQuery: remove promiseCache, work around race condition Nov 16, 2023

phryneas requested a review from jerelmiller November 16, 2023 11:29

phryneas commented Nov 16, 2023

View reviewed changes

phryneas added 3 commits November 16, 2023 13:58

update current[0] correctly

532505a

change key to empty object

50c2149

we only care about identity, not about contents

Merge branch 'pr/remove-useSuspenseQuery-promiseCache' into pr/remove…

6dd943a

…-useBackgroundQuery-promiseCache

phryneas added this to the MemoryAnalysis milestone Nov 16, 2023

phryneas added 2 commits November 21, 2023 15:33

move secondIfNewerFulfilledOrFirst call into unwrapQueryRef

c9c3177

remove unused import

4a5c3e2

phryneas requested a review from alessbell November 28, 2023 18:27

jerelmiller reviewed Nov 30, 2023

View reviewed changes

phryneas added 4 commits November 30, 2023 16:39

PR feedback

083af14

more PR feedback

3da56f0

review feedback

2cad580

re-add delay to test

346e5e8

phryneas marked this pull request as ready for review November 30, 2023 17:26

phryneas requested a review from jerelmiller November 30, 2023 17:27

phryneas and others added 2 commits November 30, 2023 18:31

add more mock delays

fc66f12

Separate withSequence from createFulfilledPromise and createRejectedP…

1e3297e

…romise

jerelmiller approved these changes Nov 30, 2023

View reviewed changes

github-actions bot added the auto-cleanup 🤖 label Nov 30, 2023

jerelmiller reviewed Nov 30, 2023

View reviewed changes

Remove need for sequencing on promises by returning newer promise whe…

a6e421a

…n its fulfilled

jerelmiller approved these changes Dec 1, 2023

View reviewed changes

phryneas added 4 commits December 1, 2023 10:52

Merge remote-tracking branch 'origin/release-3.9' into pr/remove-useB…

53871a1

…ackgroundQuery-promiseCache

useLoadableQuery: remove promiseCache

82bd4ae

remove second argument to wrapQueryRef

f8e0e05

chores

10175b5

phryneas requested a review from a team as a code owner December 1, 2023 10:00

Base automatically changed from pr/remove-useSuspenseQuery-promiseCache to release-3.9 December 1, 2023 10:32

phryneas and others added 7 commits December 1, 2023 11:36

Merge remote-tracking branch 'origin/release-3.9' into pr/remove-useB…

b248d6c

…ackgroundQuery-promiseCache

fix most tests by adding delays

4a413c3

Clean up Prettier, Size-limit, and Api-Extractor

8010b0c

add skipNonTrackingRenders to profiler

c150398

to account for "empty" renders that don't actually render any components we're interested in testing

add comments to "phantom render" tests

d07aa6a

Merge branch 'release-3.9' into pr/remove-useBackgroundQuery-promiseC…

1eef04f

…ache

Clean up Prettier, Size-limit, and Api-Extractor

4bde5a0

phryneas merged commit db5f5fd into release-3.9 Dec 1, 2023
22 checks passed

phryneas deleted the pr/remove-useBackgroundQuery-promiseCache branch December 1, 2023 18:13

jerelmiller mentioned this pull request Dec 1, 2023

Update useLoadableQuery to remove promiseCache #11402

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`useBackgroundQuery`: remove `promiseCache`, work around race condition #11366

`useBackgroundQuery`: remove `promiseCache`, work around race condition #11366

phryneas commented Nov 16, 2023 •

edited

Loading

changeset-bot bot commented Nov 16, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

phryneas Nov 16, 2023

jerelmiller Nov 30, 2023

phryneas Nov 16, 2023

phryneas Nov 16, 2023

jerelmiller Nov 30, 2023

jerelmiller left a comment

jerelmiller Nov 30, 2023

phryneas Nov 30, 2023

jerelmiller Nov 30, 2023

phryneas Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller Nov 30, 2023

phryneas Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller Nov 30, 2023

phryneas Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller Nov 30, 2023

jerelmiller left a comment

jerelmiller Nov 30, 2023 •

edited

Loading

jerelmiller left a comment

		);
		promise = secondIfNewerFulfilledOrFirst(promise, internalQueryRef.promise);

	let wrappedQueryRef = wrapped.current;
	const wrappedQueryRef = wrapped.current;

	let promise = useSyncExternalStore(
	const promise = useSyncExternalStore(

useBackgroundQuery: remove promiseCache, work around race condition #11366

useBackgroundQuery: remove promiseCache, work around race condition #11366

Conversation

phryneas commented Nov 16, 2023 • edited Loading

Checklist:

changeset-bot bot commented Nov 16, 2023 • edited Loading

⚠️ No Changeset found

github-actions bot commented Nov 16, 2023 • edited Loading

size-limit report 📦

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

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 left a comment

Choose a reason for hiding this comment

jerelmiller Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

jerelmiller left a comment

Choose a reason for hiding this comment

`useBackgroundQuery`: remove `promiseCache`, work around race condition #11366

`useBackgroundQuery`: remove `promiseCache`, work around race condition #11366

phryneas commented Nov 16, 2023 •

edited

Loading

changeset-bot bot commented Nov 16, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading

jerelmiller Nov 30, 2023 •

edited

Loading