Fix the compatibility between `useSuspenseQuery` and React's `useDeferredValue` and `startTransition` APIs #10672

jerelmiller · 2023-03-23T00:31:58Z

Major refactor to useSuspenseQuery that allows it to work with useDeferredValue and startTransition when changing variables.

There are several improvements to this re-implementation that are of note:

We no longer create and subscribe to a single ObservableQuery for each use of the hook. Instead ObservableQuery instances are created per unique query/variables combination.
These ObservableQuery instances are stored and subscribed to in an instance managed outside of React. Not having to manage the lifecycle of the query inside of the React + Suspense lifecycle meant I was able to remove some nasty hacks that plagued the previous implementation.
We have 1 less render when changing variables because we now suspend immediately when changing variables. This also makes it possible for the hook to be properly used with useDeferredValue and startTransition since these APIs require measuring whether the change results in the component suspending or not.
The networkStatus is now set properly using suspensePolicy: 'initial' when using refetch, fetchMore, or changing variables since we more reliably read the result state.

NOTE: The refetch and fetchMore functions returned from this hook still don't quite work right with startTranstion. I'm hoping to fix this in a future PR. If we can get those functions working with startTransition, we should be able to remove the suspensePolicy option from the hook since startTransition gives us the UX behavior that suspensePolicy was meant to replicate. I've opened #10676 to track this.

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-03-23T00:32:03Z

🦋 Changeset detected

Latest commit: 2ef46d3

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 · 2023-03-23T00:33:35Z

src/core/ObservableQuery.ts

@@ -134,7 +132,7 @@ export class ObservableQuery<

      // Initiate observation of this query if it hasn't been reported to
      // the QueryManager yet.
-      if (first && fetchOnFirstSubscribe) {


This refactor allowed me to get rid of this newly introduced option that was needed for my first implementation of useSuspenseQuery 🎉 . Always great to preserve the original functionality in core!

jerelmiller · 2023-03-23T00:34:18Z

src/core/ObservableQuery.ts

-  // created from `reobserve`. This function provides the original `reobserve`
-  // functionality, but returns a concast instead of a promise. Most consumers
-  // should prefer `reobserve` instead of this function.
-  public reobserveAsConcast(


Similarly, I no longer need to control the lifecycle of fetching and do not need to get the underlying concast object 🎉

jerelmiller · 2023-03-23T00:38:13Z

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

@@ -2965,16 +3276,11 @@ describe('useSuspenseQuery', () => {
      });
    });

-    expect(renders.count).toBe(5);
+    expect(renders.count).toBe(4);


This refactor means 1 less render cycle since the hook now suspends immediately when new variables are passed in rather than setting to variables and waiting for the next render to suspend 🎉

jerelmiller · 2023-03-23T00:39:58Z

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

    expect(renders.suspenseCount).toBe(1);
    expect(renders.frames).toMatchObject([
      {
        ...mocks[0].result,
        networkStatus: NetworkStatus.ready,
        error: undefined,
      },
+      {
+        ...mocks[0].result,
+        networkStatus: NetworkStatus.refetch,


When suspensePolicy is set to initial, we now get a proper networkStatus which will help users detect when a fetch might be happening in the background. This is useful if you want to provide some kind of loading indication without completely replacing the UI with a loading spinner.

jerelmiller · 2023-03-23T06:02:19Z

src/react/hooks/useSuspenseQuery.ts

-      return promise;
-    },
-    [observable]
+    (options) => subscription.fetchMore(options) as any,


I could use some help typing this correctly. I'm getting the dreaded error:

Type 'TData' is not assignable to type 'TFetchData'. 'TFetchData' could be instantiated with an arbitrary type which could be unrelated to 'TData'.

I believe the commit I pushed to this branch (aa8c9c1) should improve these types.

Never mind, that caused other type errors, so I un-pushed that commit.

jerelmiller · 2023-03-23T22:02:36Z

src/react/cache/QuerySubscription.ts

+  onDispose?: () => void;
+}
+
+export class QuerySubscription<TData = any> {


I'm not in love with the "Subscription" name here, so if someone else has a better suggestion, I'm all ears!

This class is responsible for subscribing to the ObservableQuery and caching its result/promise for use in suspense. Outsiders can "listen in" on those changes by calling its "listen" function.

jerelmiller · 2023-03-23T22:13:20Z

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

@@ -3820,7 +4261,16 @@ describe('useSuspenseQuery', () => {
    ]);
  });

-  it('applies nextFetchPolicy after initial suspense', async () => {
+  // Jerel: I cannot figure out how to properly show the functionality of


I would also appreciate some feedback/ideas on this particular comment

Try triggering the reobservation with/without nextFetchPolicy by writing to the cache in a way that results in an incomplete cache broadcast for the query in question (perhaps by calling cache.evict)?

Thanks to #9504, an ordinary cache write that produces complete results will no longer trigger a full reobservation for network-only and cache-and-network queries (even in the absence of nextFetchPolicy: "cache-first"), so it's gotten a little more difficult to see the value of nextFetchPolicy, since some of the sharp edges have been removed from the network-only and cache-and-network fetch policies.

That said, I'm not opposed to removing nextFetchPolicy from the useSuspenseQuery options, though it might still creep in via clientOptions.defaultOptions.watchQuery.nextFetchPolicy.

jerelmiller · 2023-03-23T22:21:42Z

src/react/hooks/useSuspenseQuery.ts

-          // Related to https://github.com/apollographql/apollo-client/issues/10478
-          const result = resultRef.current!;
-
-          if (


This refactor also helped me get rid of some of the weird hacks/edge cases that plagued my initial implementation 🎉

jerelmiller · 2023-03-23T23:04:36Z

/release:pr

github-actions · 2023-03-23T23:06:12Z

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

jerelmiller · 2023-03-24T15:11:35Z

Confirmed with the original reproduction that this now works as expected.

benjamn

This is a huge simplification @jerelmiller, and everything looks good to me, especially given all the tests you've added. Some comments inline.

benjamn · 2023-03-24T14:52:37Z

src/utilities/common/mergeOptions.ts

-    variables: {
+    variables: compact({
      ...(defaults && defaults.variables),
      ...options.variables,
-    },
+    }),


Small worry: this change might be removing the ability of options.variables to unset variables by explicitly passing undefined variable keys.

Funny enough, I added this to fix that problem 😄. I have a test that explicitly checks to see if I can unset a globally defined var from my query by passing undefined. Without this change, I found that the key was not properly unset like I expected it to. The value was properly set as undefined, but the key still existed.

benjamn · 2023-03-24T15:09:17Z

src/react/cache/QuerySubscription.ts

+
+    const concast = observable['concast'];
+
+    this.promise = hasDirectives(['defer'], observable.query)


This check probably needs to be more general (not just @defer, maybe a helper function), and should be cached because traversing the entire query is expensive.

Totally agree on the caching aspect since query might be the same between different instances of a QuerySubscription. I'll refactor this into a separate method outside the class that supports caching this computation.

Oh, and just to check, when you say "more general", what types of other cases are you thinking about?

The reason I need to do this check is because I want to resolve on the first chunk of data for deferred queries, which means creating my own promise here, rather than waiting for concast.promise to resolve, since that waits for all chunks before resolving. Are there other cases that you can think of where I'd want that same type of logic? None come to my mind at the moment.

Once we support @stream, those queries should probably have similar behavior to @defer (especially since you can mix @defer and @stream in the same query).

Gotcha! I figured we'd tweak this when we were ready for @stream, but I have no problem getting it ready now.

Oh I don't think we should indicate any support for @stream yet, but it would be nice to centralize this logic somewhere (maybe here, maybe a helper function) so we can easily update it whenever we add support for @stream or other features that deliver incremental chunks (and so benefit from the early-resolving promise).

Wrapped this helper with optimism's wrap to cache the result, and moved the function out to its own helper function (2986713). Once we need this logic elsewhere, it should be trivial to pull over to a different file.

benjamn · 2023-03-24T15:16:08Z

src/react/cache/QuerySubscription.ts

+  private handleNext() {
+    const result = this.observable.getCurrentResult();
+
+    if (!equal(this.result, result)) {


What's the status of potentially not calling getCurrentResult here?

These deep equality checks have a tendency pile up (redundant/costly). In this case we're ignoring the result passed to the next handler, which was already deep-equality-checked by ObservableQuery#isDifferentFromLastResult. Do we really need this one too?

I'll try it with the value passed to the next handler and see what tests break. I know useQuery currently has problems with that value and I expected to see the same, though I didn't try it again with this refactor (not sure why, now that I think about it). Will report back with results.

After trying this where I'm using the value passed to handleNext, this actually seemed to catch a few issues with incorrect networkStatus values set in the hook, so in general I think this is a mostly positive change. The one difference I see between the two now is the case where refetching with an error after a successful initial fetch won't keep the previous data value, but rather unset it completely (using errorPolicy: 'all' in this case).

I'll try and gauge what it would take to fix this in core (I assume we'd count this as a bug). For now I might try and fix in the QuerySubscription itself and come back to it in a separate PR so we can call out that change appropriately without being buried in this refactor.

I updated this in f5e3ba7 which actually helped me find some incorrect networkStatus values returned from the hook when an error was encountered. This also let me remove the deep equality check in f5e3ba7.

benjamn · 2023-03-24T15:26:36Z

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

@@ -3820,7 +4261,16 @@ describe('useSuspenseQuery', () => {
    ]);
  });

-  it('applies nextFetchPolicy after initial suspense', async () => {
+  // Jerel: I cannot figure out how to properly show the functionality of


Try triggering the reobservation with/without nextFetchPolicy by writing to the cache in a way that results in an incomplete cache broadcast for the query in question (perhaps by calling cache.evict)?

Thanks to #9504, an ordinary cache write that produces complete results will no longer trigger a full reobservation for network-only and cache-and-network queries (even in the absence of nextFetchPolicy: "cache-first"), so it's gotten a little more difficult to see the value of nextFetchPolicy, since some of the sharp edges have been removed from the network-only and cache-and-network fetch policies.

benjamn · 2023-03-24T16:08:16Z

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

@@ -3820,7 +4261,16 @@ describe('useSuspenseQuery', () => {
    ]);
  });

-  it('applies nextFetchPolicy after initial suspense', async () => {
+  // Jerel: I cannot figure out how to properly show the functionality of


That said, I'm not opposed to removing nextFetchPolicy from the useSuspenseQuery options, though it might still creep in via clientOptions.defaultOptions.watchQuery.nextFetchPolicy.

benjamn · 2023-03-24T16:25:47Z

config/bundlesize.ts

-const gzipBundleByteLengthLimit = bytes("34.1KB");
+const gzipBundleByteLengthLimit = bytes("34.01KB");


Love to see a bundlesize reduction!

#10672 (comment)

…new query

…part of the key

… request is finished

…getCurrentResult()

…s returned with error policy as all or ignore

…at checks for stream and caches result

…nt does not subscribe to updates in a given time

jerelmiller added 🐞 bug 👩‍🏭 refactor ⏲️ react-suspense labels Mar 23, 2023

jerelmiller changed the title ~~Update useSuspenseQuery to work with useDeferredValue and startTransition~~ Fix the compatibility between useSuspenseQuery and React's useDeferredValue and startTransition APIs Mar 23, 2023

jerelmiller commented Mar 23, 2023

View reviewed changes

jerelmiller force-pushed the use-deferred-value-diff-approach branch from 8ea6baa to 211a8ac Compare March 23, 2023 06:07

jerelmiller linked an issue Mar 23, 2023 that may be closed by this pull request

useDeferredValue doesn't work with useSuspenseQuery #10534

Closed

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

Version Packages (alpha) #10675

Closed

jerelmiller commented Mar 23, 2023

View reviewed changes

jerelmiller force-pushed the use-deferred-value-diff-approach branch from 4f92b47 to 29581f3 Compare March 23, 2023 22:25

jerelmiller marked this pull request as ready for review March 23, 2023 22:38

jerelmiller requested review from alessbell, benjamn and phryneas March 23, 2023 22:38

benjamn approved these changes Mar 24, 2023

View reviewed changes

benjamn reviewed Mar 24, 2023

View reviewed changes

benjamn added a commit that referenced this pull request Mar 24, 2023

Improve FetchMoreFunction to avoid generic type parameter mismatch.

e2a05a1

#10672 (comment)

benjamn added a commit that referenced this pull request Mar 24, 2023

Improve FetchMoreFunction to avoid generic type parameter mismatch.

aa8c9c1

#10672 (comment)

benjamn force-pushed the use-deferred-value-diff-approach branch 2 times, most recently from aa8c9c1 to 1a65cbc Compare March 24, 2023 17:12

jerelmiller and others added 23 commits March 28, 2023 10:59

Don't export the new wrapPromiseWithState utility publicly

ac0f50e

Reduce bundlesize

36f41f3

Add test to validate that changing clients will trigger suspense and …

e26594f

…new query

Move data type for simple query case out to global scope

f51bb73

Unify SuspenseCache and SuspenseQueryCache together by making client …

36336a8

…part of the key

Minor adjustment to comment

4d0a5d1

Make onDispose a private function

0016f92

Rename helper function to avoid thinking its a hook

b7e42d8

Reduce bundlesize to 34.01KB

c818962

Add comment about setting previous data

6e90b29

Expand FetchMoreFunction type to fix TFetchData type mismatch.

3bb783d

Create a new isNetworkRequestSettled function to determine if network…

a8842b8

… request is finished

Use the value passed to handleNext rather than relying on observable.…

68b8276

…getCurrentResult()

Remove unneeded deep equality check when getting next result

9d6dfdb

Add 2 tests to validate cache updates are propagated after an error i…

f03ec36

…s returned with error policy as all or ignore

Use ObservableSubscription type for subscription object

dc5d1a0

Extract function to determine custom wrapped promise out to method th…

159e5da

…at checks for stream and caches result

Remove support for nextFetchPolicy in options

eb69d96

Add a timer that will auto dispose of the subscription if the compone…

84eb2b4

…nt does not subscribe to updates in a given time

Remove errant imported type

40c7659

Update bundlesize to 34.14KB

01d443e

Override nextFetchPolicy in watchQueryOptions

6ec4f6a

Update changeset to mention removal of nextFetchPolicy

2ef46d3

jerelmiller force-pushed the use-deferred-value-diff-approach branch from 1650756 to 2ef46d3 Compare March 28, 2023 16:59

jerelmiller merged commit 932252b into release-3.8 Mar 28, 2023

jerelmiller deleted the use-deferred-value-diff-approach branch March 28, 2023 19:25

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

Version Packages (alpha) #10670

Merged

This was referenced Mar 28, 2023

useDeferredValue doesn't work with useSuspenseQuery #10534

Closed

Add a queryKey option for useSuspenseQuery #10700

Merged

github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the compatibility between `useSuspenseQuery` and React's `useDeferredValue` and `startTransition` APIs #10672

Fix the compatibility between `useSuspenseQuery` and React's `useDeferredValue` and `startTransition` APIs #10672

jerelmiller commented Mar 23, 2023 •

edited

Loading

changeset-bot bot commented Mar 23, 2023 •

edited

Loading

jerelmiller Mar 23, 2023

jerelmiller Mar 23, 2023

jerelmiller Mar 23, 2023

jerelmiller Mar 23, 2023

jerelmiller Mar 23, 2023

benjamn Mar 24, 2023

benjamn Mar 24, 2023

jerelmiller Mar 23, 2023

jerelmiller Mar 23, 2023

benjamn Mar 24, 2023

benjamn Mar 24, 2023

jerelmiller Mar 23, 2023

jerelmiller commented Mar 23, 2023

github-actions bot commented Mar 23, 2023

jerelmiller commented Mar 24, 2023

benjamn left a comment

benjamn Mar 24, 2023

jerelmiller Mar 24, 2023

benjamn Mar 24, 2023

jerelmiller Mar 24, 2023

jerelmiller Mar 24, 2023

benjamn Mar 24, 2023

jerelmiller Mar 24, 2023

benjamn Mar 24, 2023

jerelmiller Mar 25, 2023

benjamn Mar 24, 2023

jerelmiller Mar 24, 2023

jerelmiller Mar 24, 2023 •

edited

Loading

jerelmiller Mar 24, 2023

benjamn Mar 24, 2023

benjamn Mar 24, 2023

benjamn Mar 24, 2023


		const concast = observable['concast'];

		this.promise = hasDirectives(['defer'], observable.query)

		const gzipBundleByteLengthLimit = bytes("34.1KB");
		const gzipBundleByteLengthLimit = bytes("34.01KB");

Fix the compatibility between useSuspenseQuery and React's useDeferredValue and startTransition APIs #10672

Fix the compatibility between useSuspenseQuery and React's useDeferredValue and startTransition APIs #10672

Conversation

jerelmiller commented Mar 23, 2023 • edited Loading

Checklist:

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

🦋 Changeset detected

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 commented Mar 23, 2023

github-actions bot commented Mar 23, 2023

jerelmiller commented Mar 24, 2023

benjamn 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

jerelmiller Mar 24, 2023 • 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

Fix the compatibility between `useSuspenseQuery` and React's `useDeferredValue` and `startTransition` APIs #10672

Fix the compatibility between `useSuspenseQuery` and React's `useDeferredValue` and `startTransition` APIs #10672

jerelmiller commented Mar 23, 2023 •

edited

Loading

changeset-bot bot commented Mar 23, 2023 •

edited

Loading

jerelmiller Mar 24, 2023 •

edited

Loading