Consider the behavior of useSuspenseQuery when non client/query/variables options change

[jerelmiller]

With the refactor for useSuspenseQuery in #10672, we lost the ability to update the watchQueryOptions on the ObservableQuery when changed between renders (i.e. new props passed into the hook). Before the refactor, changes to query and/or variables would result in passing the whole watchQueryOptions to reobserve, thereby updating ObservableQuery with those options. With the refactor, the ObservableQuery lifecycle is now handled outside React with no mechanism to change watchQueryOptions. It appears none of this behavior was tested in the current test suite. That might be ok because this gives us an opportunity to determine what the correct behavior should be (NOTE: client, query, and variables are not included as changes to these are properly propagated with refetches).

Because we are using React Suspense, there are additional considerations to take into account. Traditionally watchQueryOptions are updated through the method ObservableQuery.reobserve, which has the potential to kick off a new network request (for example, when fetchPolicy changes to network-only or no-cache). In a Suspense world, new network requests have the possibility of re-suspending the component. Depending on the granularity of the user's Suspense boundary, this could be jarring to an end user.

To contrast, useQuery uses this technique to update options. It however has 2 benefits: 1. It supports notifyOnNetworkStatusChange as an option, which when false will prevent rerenders for new loading states and 2. Loading states are done inline, so even when notifyOnNetworkStatusChange is true, it limits the blast radius of the loading state.

I see a few paths forward:

1. We could maintain the behavior where changing options could result in a new network request, thereby suspending the component (there is some nuance here, such as changing to a cache-only fetchPolicy wouldn't fetch, but for the sake of the argument, I'm choosing to keep it simple). With the technique, we'd likely want to recommend using React's startTransition API for users that want to avoid suspending and show the existing UI until loaded.
2. We could silently update options on the ObservableQuery, which would take effect for results going forward. For example, changes to the errorPolicy would only affect future requests.
3. We ignore changes to options entirely. The options set on ObservableQuery from the initial render are set in stone and changes to them do not affect anything going forward. This is the most simple, but has the potential to confuse our users.

To add more nuance to the discussion, this may be very option dependent. It's possible we could use a combination of all 3 paths mentioned above and choose which technique to apply to each option. For example, should changing the errorPolicy between renders cause the query to refetch? My take is no: this should only affect network requests going forward, so this feels like something that should be silently updated. What about changing the fetchPolicy? I could see the argument where this could warrant a potential refetch.

I think it would be helpful to go through each option and determine how changes to that option would affect the behavior.

Supported options

Here is the list of currently supported options. For brevity, I have omitted client, variables, and suspenseCache as changes to these options propagate as intended: They will execute a new request when changed.

NOTE: This list may change between now and final release, so the list captures the currently supported options at the time of this writing. I'll do my best to keep this updated if we add/remove options before this issue is completed.

• errorPolicy
• context
• canonizeResults
• returnPartialData
• refetchWritePolicy
• fetchPolicy

[phryneas]

We are creating a new API here, so let's consider what would result in the least amount of bugs & confusion.

I suggest removing the arguments to the refetch/fetchMore/subscribeToMore functions.

I might even suggest removing the fetchMore/subscribeToMore functions? 🤔 I believe these currently can cause problems when two components start from the same point and one of the two starts "moving variables"?

These methods always create a ton of ambiguity on how things should work with them, and they are the source of many conflicts - and as a result, of bugs.

Instead, we could try to embrace React here: if users want to change their variables/options/etc., they can maintain them themselves in a useState/useReducer.

We could even offer a helper hook for that.

So, we would end up with scenarios where users would have very granular control over what happens and when.
We would have to make sure how different scenarios would work, though.

In light of that, we could revisit the question on "when and how to refetch" that you posed - and the answer could be one of:

1. whenever variables or relevant options change
2. only user-initiated

I believe 2. would be problematic with variables passed in from a parent component, so I suggest we go with 1.

The question of "suspend or not" is also an interesting one. I'd almost say "always suspend" - this is useSuspenseQuery, and the user has control over whether or not this should show a spinner by using useTransition.

[jerelmiller]

I suggest removing the arguments to the refetch/fetchMore/subscribeToMore functions.

I might even suggest removing the fetchMore/subscribeToMore functions?

I've actually considered this as well, however I haven't fully committed to this yet for a few reasons. I'll address both, but let me take them one function at a time.

refetch
This definitely seems like the best candidate of the 3 functions since you can just pass in new variables to the hook to fetch with those variables. The biggest difference however is that refetch will guarantee a network request, where updating variables may or may not fetch (depending on the fetchPolicy). Being able to force a network request for different variables might be ideal.

It appears Relay does have some form of a refetch with new variables, so this pattern isn't totally foreign.

fetchMore
This is the primary API for pagination so I hesitate to remove this. Without this, I'm not sure we have another way to load other pages of data. For example, the Spotify app relies on pagination to provide infinite scroll for tracks. You can see this used here. For this reason, I think this function both needs to be exported and needs to accept arguments so that additional pages of data can be loaded.

subscribeToMore
I initially did not return this function from useSuspenseQuery, but I found this is useful for subscriptions. The Spotify API uses this function to first load the playback state, then subscribe to changes via a subscription that will update the query to ensure the playback state is always in sync with Spotify. I found this to be useful.

This function is pretty low overhead and it doesn't interact with Suspense anyways. There is no "loading" state here, rather the connection is either opened and listening for subscription payloads or its not.

---

I believe these currently can cause problems when two components start from the same point and one of the two starts "moving variables"

Would you be able to provide an example here? I'd like to understand this a bit more. Are you thinking its two components using data from a parent useSuspenseQuery? Or two components that have their own useSuspenseQuery, but use the same query/variables?

---

These methods always create a ton of ambiguity on how things should work with them

I agree, it causes some interesting situations. One of the examples that came to mind is what happens if you refetch with new variables, then update fetchPolicy. Should it refetch? And if so, should it refetch with the original variables or the most recently refetched variables? These are definitely problems we'll need to test for and solve.

That being said, it makes me wonder how much of this problem also exists in other hooks such as useQuery. useQuery will call reobserve when any watch query option changes, so its possible we have this same problem over there. Perhaps this is something to explore a bit deeper.

---

If users want to change their variables/options/etc., they can maintain them themselves in a useState/useReducer.

I absolutely agree. I want users to be able to leverage any and all tools that React provides (startTransition/useTransition) with our hook in combination with variables/options/etc. No reason we shouldn't encourage use of these features.

The main reason I opened this issue though is in the most recent refactor, regardless of how options are stored and passed to the hook, variables, client, and suspenseQuery are the only options that will actually do anything when changed. If any of the other options change, the changes are simply ignored, and I'm not sure that's the behavior we want. There certainly could be an argument made that we should consider leaving this behavior as well, but I suspect users might find this confusing.

---

We could even offer a helper hook for that.

I'd be curious what this might look like. I'm not fully convinced we need a separate hook, but I'd like to understand your idea here more before I shrug this off. Perhaps some code samples?

---

I'd almost say "always suspend" - this is useSuspenseQuery, and the user has control over whether or not this should show a spinner by using useTransition.

This x1000. I absolutely agree. No reason we should avoid suspending when loading data. Its the reason I'd like to get #10676 done so that I can remove that suspensePolicy option.

Really I'd just like to figure out if there are any other options that, when changed, should cause the hook to fetch, or whether these options are "silently" applied and will be set for behavior moving forward (for example, any fetch after changing errorPolicy should adhere to the new errorPolicy, but changing the errorPolicy shouldn't result in a fetch itself).

---

Anyways, thanks so much for these thoughts! Would love to get a bit more info from you on a couple of your ideas.

[jerelmiller]

We are creating a new API here

I meant to comment on this as well, but you're absolutely right. This is actually a very important point and I don't want to lose sight of this. I think we should have the freedom to APIs that may or may not line up with other parts of Apollo since this is a totally new paradigm.

I tended to err on the side of maintaining a familiarity with useQuery, but I'm not sure thats a compelling enough reason itself if maintaining that consistency ends up with confusion and bugs because of this new paradigm.

[phryneas]

Phew, tons to unpack here :)

refetch / changing variables

This definitely seems like the best candidate of the 3 functions since you can just pass in new variables to the hook to fetch with those variables. The biggest difference however is that refetch will guarantee a network request, where updating variables may or may not fetch (depending on the fetchPolicy). Being able to force a network request for different variables might be ideal.

My thought was actually to change the refetch behaviour in a way that it would trigger a rerender and then guarantee a refetch during the next render. Due to Reacts auto-batching that would allow you to do something like

setVariables(newVars);
refetch(); //maybe rename it to `queueRefetch`?

and it would just go the "normal React way", while keeping the "state-keeping" out of our implementation.

Our implementation would probably need a useState and a useRef here to make sure the rerender happens, but the rerender only triggers one refetch, not multiple, during Suspense/StrictMode.

Something like

const lastRefetch = useRef();
const [nextRefetch, forceRefetch] = useReducer((x) => x + 1);
if (lastRefetch.current !== nextRefetch) {
  lastRefetch.current = nextRefetch;
  // trigger refetch
}

fetchMore

This is the primary API for pagination so I hesitate to remove this. Without this, I'm not sure we have another way to load other pages of data. For example, the Spotify app relies on pagination to provide infinite scroll for tracks. You can see this used here. For this reason, I think this function both needs to be exported and needs to accept arguments so that additional pages of data can be loaded.

I had naively assumed that even with a refetch call, the result would be merged in, as that would done mostly by typePolicies, not fetchMore - but it's very possible that there is a ton of additional value here that I might be missing.

subscribeToMore

This function is pretty low overhead and it doesn't interact with Suspense anyways. There is no "loading" state here, rather the connection is either opened and listening for subscription payloads or its not.

This would be my main question: how much overhead is this to have in and maintain it in the future? If it's not a lot, okay, but please make sure that it's not a starter for feature creep. Worst case we have one more hook, but didn't overload useSuspenseQuery.

Would you be able to provide an example here? I'd like to understand this a bit more. Are you thinking its two components using data from a parent useSuspenseQuery? Or two components that have their own useSuspenseQuery, but use the same query/variables?

I am thinking of two independent components calling both useSuspenseQuery with the exact same arguments, so they get the same Subscription instance - and then one of them doing a .refetch with different variables. Will it change the variables for both components?

I'd be curious what this might look like. I'm not fully convinced we need a separate hook, but I'd like to understand your idea here more before I shrug this off. Perhaps some code samples?

Nothing specific to be honest. More a reminder that when we decide to holding "mutating internal state" within useSuspenseQuery, we might see some patterns coming up in userland code - and that we then could think of providing helpers for that.

I meant to comment on this as well, but you're absolutely right. This is actually a very important point and I don't want to lose sight of this. I think we should have the freedom to APIs that may or may not line up with other parts of Apollo since this is a totally new paradigm.

I guess that's the main point of my comment. I cannot really tell you a lot on the initial question, what should refetch when, as I'm just not deep enough into that yet - but I can very well try to play devil's advocate and as "how much of this do we need and how much of it is just inherited?" :)

[jerelmiller]

I am thinking of two independent components calling both useSuspenseQuery with the exact same arguments, so they get the same Subscription instance - and then one of them doing a .refetch with different variables. Will it change the variables for both components?

FYI I just opened a PR (#10700) that addresses this issue. Currently yes, a refetch would affect both, but now you can use a queryKey to ensure you have unique instances.

[jerelmiller]

Done in #11025