useSuspenseQuery

[jerelmiller]

Relates to #10231 and #10286

Adds a new useSuspenseQuery hook to allow React apps to use Apollo with React's suspense feature. For now, this hook is exported as an experimental hook to make it clear that we are looking for feedback and iterating on it.

This implementation in this PR focuses on baseline support for React suspense with Apollo. This does not yet support @defer directives, has not been tested with SSR, and does not provide a solution for the render-as-you-fetch pattern . These will be covered in future PRs and alphas.

Usage

1. Ensure your <ApolloProvider /> has a SuspenseCache

import { SuspenseCache } from '@apollo/client';

// ...

<ApolloProvider client={client} suspenseCache={new SuspenseCache()}>
  <App />
</ApolloProvider>

2. Write suspenseful queries

import { Suspense } from 'react';

import { 
  gql,
  // useSuspenseQuery is exported as experimental. 
  // Use an alias to make it more friendly to use
  useSuspenseQuery_experimental as useSuspenseQuery 
} from '@apollo/client';

const QUERY = gql`
  query($id: ID!) {
    user(id: $id) {
      id
      name
    }
  }
`;

function MyParentComponent() {
  return (
    <Suspense fallback="Loading...">
      <MyComponent />
    <Suspense>
  );  
}

function MyComponent() {
  const { data } = useSuspenseQuery(QUERY, { 
    variables: { id: 1 }
  });

  return (
    <div>
      {/* Just use `data`! */}
      {data.user.name}
    </div>
  );
}

Design

useSuspenseQuery has been modeled after useQuery to make the hook feel familiar to those that use Apollo, though it is a much more scaled down version. I did not add support for all options nor return the same set of values you get from the useQuery hook. My goal is to start with a small surface area and add options/return values as we get feedback through our alpha release process.

Supported options

• variables
• client
• errorPolicy
• context
• returnPartialData
• canonizeResults
• fetchPolicy (all but cache-only and standby. See the RFC for more info)
• nextFetchPolicy
• refetchWritePolicy

Returned properties

• data
• error
• refetch
• fetchMore

Checklist:

• 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

[jerelmiller]

I'm getting some test failures in this branch for unrelated tests. I rebased with release-3.8 recently and this error started popping up. I am able to reproduce this locally using the same version of Node that we use in CircleCI on the release-3.8 branch. What's interesting is that I can only seem to recreate the error if I run the whole test suite. If I run just the file with the failure, all tests pass. We'll likely need to correct this on the release-3.8 branch itself.

[jerelmiller]

After some investigation for the test flake on this issue, my guess is this comes down to timers. In this test, we are checking that polling works as expected. Our expectations use waitForNextUpdate() to run the next set of expectations, but these utilities also rely on setTimeout to check that the result has changed every so often. My theory is that sometimes the polling functionality fires "too quickly" for waitForNextUpdate to see it, therefore the test assertions check data that is now already stale, resulting in a failure.

I wanted to at least document my findings. I have been able to run the whole test suite successfully, though its very intermittent. Hopefully I can get a run that passes so we can merge this PR when reviews are finished.

[changeset-bot]

🦋 Changeset detected

Latest commit: 5212938

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	Minor

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]

This comment is now out-of-date with the introduction of the fetchOnFirstSubscribe option. I'll amend this comment to be more accurate with the change.

[alessbell]

Super excited for this! 🚀🎉🙌

[pkellner]

Does this PR take advantage of the new "use" proposal currently in alpha from the React team?

Also, I notice you are using a cache. I understand that currently, that is one of the pieces missing from the React 229 PR. That is, without having a consistent cache, Suspense has problem with any data persistence between client and server. Is this PR strictly meant for client only testing?

reactjs/rfcs#229

[jerelmiller]

Does this PR take advantage of the new "use" proposal currently in alpha from the React team?

@pkellner no it does not. We will likely migrate to it when it's ready and stable, but for now we're aiming to get this in front of as many devs using Apollo Client as we can. We are keeping on eye on that RFC to see how things evolve and if it will make sense for us to migrate to use when the time comes.

Is this PR strictly meant for client only testing?

For now, yes! As mentioned in the PR description:

This... has not been tested with SSR

This may work on the server with React 18, but we do not yet have any kind of cache hydration mechanism for it, so some of the advantages you get with SSR are moot. As mentioned in the "Release strategy" section of the RFC, formal SSR support (with cache hydration) will be coming in a future alpha.

I suspect the implementation will evolve quite a bit between this PR and our GA release (sometime early 2023) as we learn and get more feedback on our Suspense implementation. In the mean time, if you'd like to try out at least the base-level implementation, I'd welcome any and all feedback you have! I saw you commented quite a bit on the original issue so I know you've been thinking about this a lot already.

[laverdet]

Thank you for the ongoing work with this feature.

I believe this implementation suffers from the waterfall issue I pointed out in this comment #10231 (comment). Is it possible to dispatch 2 queries within the same suspense boundary and have them run at the same time?

[jerelmiller]

Hey @laverdet!

You're correct in that this implementation does suffer from the waterfall issue that you mentioned. After some thought and discussion, I purposefully made it this way. I updated the RFC a while back to explain this behavior. Check out the "Notable changes in behavior from useQuery" section, specifically the bullet point titled "Multiple useSuspenseQuery hooks in the same component will result in a request waterfall".

Check out my comment that explains why I decided to go this route. Specifically the observation I made about the new use hook:

In fact, later in the proposal, this is stated:

If a promise passed to use hasn't finished loading, use suspends the component's execution by throwing an exception. When the promise finally resolves, React will replay the component's render. During this subsequent attempt, the use call will return the fulfilled value of the promise.

I understand this to mean that the component would suspend immediately for pending promises. Given this proposal from the React team (understanding this could change between now and release), I wonder if we should do the same (which would revert this proposal to an earlier version).

In my research on other libraries' implementations, it seems this was the common pattern: suspend immediately when the data is pending. To keep from being too "magic", I figured this was the best way forward and recommend splitting the queries between 2 components.

---

As an FYI, we do plan on enabling the render-as-you-fetch pattern with useBackgroundQuery in an upcoming alpha that will work with the suspense cache to allow you to prefetch data. In this case, useBackgroundQuery won't suspend itself, but will start loading the data immediately. If that data is still pending by the time a useSuspenseQuery hits, that hook will suspend.

We'll have more info on useBackgroundQuery and how it will play into this vision, but for now this comment paints the best picture for how we plan to make this work together. I'm hoping this will alleviate the need for needing to run 2 useSuspenseQuery hooks in the same component.

[laverdet]

Sorry for not responding in more detail to the RFC comment. I'm not sure if you found it, but there's actually a lot more context in the React RFC PR: reactjs/rfcs#229

Dan Abramov had this example code in his reply here:

const promise1 = fetchData1()
const promise2 = fetchData2()
// these are already parallel by now!
const data1 = use(promise1)
const data2 = use(promise2)

This is briefly touched on in the RFC under: "Caveat: Data requests must be cached between replays".

The idea is that suspense-capable loaders would return referentially-stable thenables. A transitional (pre-use) implementation might look like:

interface WithSuspense<Type> extends PromiseLike<Type> {
	get(): Type;
}

function makeSuspense<Type>(promise: Promise<Type>): WithSuspense<Type> {
	let resolved = false;
	let rejected = false;
	let result: unknown;
	promise.then(value => {
		resolved = true;
		result = value;
	}, error => {
		rejected = true;
		result = error;
	});
	return {
		get() {
			if (resolved) {
				return result as Type;
			} else if (rejected) {
				throw result;
			} else {
				throw promise;
			}
		},
		then(onfulfilled, onrejected) {
			return promise.then(onfulfilled, onrejected);
		},
	};
}

With this implementation we'd be able to do something like:

const result1 = useSuspenseQuery(...);
const result2 = useSuspenseQuery(...);
const data1 = result1.data.get();
const data2 = result2.data.get();

Then after (if) use ships:

const result1 = useSuspenseQuery(...);
const result2 = useSuspenseQuery(...);
const data1 = use(result1.data);
const data2 = use(result2.data);

[jerelmiller]

@laverdet ah I see what you're getting at. I'd like to better understand the use case for multiple queries in a single component to think about this some more. While I know some developers do this, I'm wondering what this enables vs splitting up queries into multiple components.

---

FWIW, I see useSuspenseQuery on its own akin to useLazyLoadQuery from Relay. Note how the hook suspends the component, but there is no external promise passed into it, nor is there any kind of unwrapping of the returned value to extract the data. You just use the data directly. Being the "vetted" solution by the React team that also suffers from the same waterfall issue you mention, it seemed like there are situations where this trade-off was acceptable. I know this approach is less desirable and I=in fact, I see the waterfall issue mentioned in those docs:

Hook used to fetch a GraphQL query during render. This hook can trigger multiple nested or waterfalling round trips if used without caution, and waits until render to start a data fetch (when it can usually start a lot sooner than render), thereby degrading performance. Instead, prefer usePreloadedQuery.

When we implement it, I see useBackgroundQuery + useSuspenseQuery similar to the way Relay does useQueryLoader + usePreloadedQuery. In this way, you should be able to execute multiple useBackgroundQuerys in a single component and have other components read/suspend those queries once they render. Like Relay, we will likely recommend this approach, since it seems the React team is pushing for this type of UX with suspense.

Would this approach/pattern address the solution you're looking for?

---

To be transparent, I see the approach of returning the data directly (no unwrapping) as a bit simpler since you can just use it right away with no additional lines of code to unwrap the value. I chose that deliberately as it seems to be the common pattern around many suspense-enabled hooks in the wild. I'm also banking on the fact that it's likely much rarer that you use multiple queries in a single component, hence why I chose the simpler approach. I'm banking on the vision of useBackgroundQuery + useSuspenseQuery helps with more of the parallel data loading patterns.

I definitely hope you provide feedback as we get more of these alphas out! Once we get more of the full Suspense vision completed, I think we'll be able to enable a lot more powerful patterns.

[laverdet]

Idk, it happens sometimes. Maybe this is a component that thought some data would be preloaded but actually it wasn't and now it needs to suspend. I don't think the code noise is too bad with an explicit .get() or use(...). Usually you need to break these destructuring constructs out into another line anyway because of the optional chaining required in most GraphQL endpoints:

const { data } = useQuery(...);
const myObject = data?.myQuery?.myObject;

In this case it would be:

const { data } = useSuspenseQuery(...);
const myObject = data.get()?.myQuery?.myObject;

The biggest problem I have with this implementation is that we can't put the suspense toothpaste back into the tube once you've already squeezed it out for us. If developers are truly unable to burden the cognitive load of .get() then it's a 1 line helper to unwrap it. On the other hand, if we do need 2 queries to run at the same time then there's going to be a lot of hoops to jump through.

In the (explicitly outdated, but yet to be replaced) React Suspense docs they apply a similar pattern:

const resource = fetchProfileData();
// [...]
function ProfileDetails() {
  // Try to read user info, although it might not have loaded yet
  const user = resource.user.read();
  return <h1>{user.name}</h1>;
}

[jerelmiller]

Hey @laverdet 👋 !

Sorry for my slow reply. Apparently I forgot to actually submit the comment I had typed up reply to you 😬

The biggest problem I have with this implementation is that we can't put the suspense toothpaste back into the tube once you've already squeezed it out for us.

I totally understand your point here. I agree with your point about wanting to parallelize as much as possible. I'll just reiterate that I believe useBackgroundQuery is going to to be the resource that will allow this to happen.

I'd love to revisit this topic once we have a more full picture of the suspense story in Apollo and get your feedback at that point.

If developers are truly unable to burden the cognitive load of .get() then it's a 1 line helper to unwrap it.

I hear you and agree that its not a lot to learn or read. I'll offer this point:

Something that I've always loved about Apollo's approach from the very first time I used it is it always felt like "plain JavaScript". A while ago I had a chance to work on a greenfield project for the company I was with at the time and we had a chance to pick our tech stack. We had settled on GraphQL and it came time to choose what GraphQL client we wanted to use. Apollo and Relay were the 2 big offerings at that time.

We had initially decided to use Relay, but after a time, it got a bit difficult to onboard new people with all the concepts that had to be learned to use it. Relay used a lot of opaque objects that you had to understand or pass around. After some time, I took a 2nd look at Apollo. Using Apollo, I really appreciated that I was working with plain objects and didn't have to understand opaque types to get the best out of the client.

Sure, .get() is not much to type or understand, but this starts to feel like I'm working with opaque types again and not with the "plain JavaScript" objects that I've come to know and love with Apollo. I'd like as best as possible to keep that same spirit with the new Suspense related behaviors.

Hope that helps! Again, I sincerely appreciate the feedback. useSuspenseQuery is just the first step and we've got more plans for the whole Suspense-related picture. As I said, I'd love to get more of your feedback once we have the bigger picture in place and I'm hoping it helps satisfy the use-cases you're after. Stay tuned!

[laverdet]

useBackgroundQuery will be nice in some cases but I think users are going to run into silent over-fetch hazards with it. You'll have a background query specified somewhere to prime a useSuspenseQuery elsewhere but when the consumer of the data gets moved the background query will remain. It's one of those things that people will either not notice or just say "oh wow my mistake" but this will be a natural consequence of the design.

I gave useSuspenseQuery a spin 2 weeks ago but it was a non-starter without the skip parameter. For example in the case we want to fetch a relationship between a user and some object but the user may be logged out. In this case we'd do useQuery(GetRelationship, { variables: { userId: userId! }, skip: !userId }). Naturally it could probably be refactored into a conditional subcomponent but that can sometimes be tough. We ended up moving forward with an internal implementation of useSuspenseQuery that handles that case. The way I did it is by making the query argument nullable. In the case query is undefined the hook just returns nothing. Then the code looks something like:

const { query, variables } = userId ? { query: GetRelationship, variables: { userId } } : {};
const result = useSuspenseQuery(query, { variables });`

[jerelmiller]

useBackgroundQuery will be nice in some cases but I think users are going to run into silent over-fetch hazards with it.

Would you be able to give me some pseudo code to sketch out what you mean here?

To use Relay as an example and the best parallel right now, I see useBackgroundQuery + useSuspenseQuery akin to useQueryLoader and usePreloadedQuery. In a library like Relay, how would this not be the case, given that its a similar pattern? Is it specifically because you've got the queryReference in that library that you pass around and gives you a clue that there is a useQueryLoader elsewhere in the app? And if I'm understanding, this could be a problem in Apollo because its an implicit relationship between these hooks vs the explicit relationship in Relay? (hope that made sense)

I gave useSuspenseQuery a spin 2 weeks ago but it was a non-starter without the skip parameter

This is really good to know! Would you be willing to open a separate issue for this as a request for the hook? I'd like to be able to track this separately.

One of the goals in this initial PR for the hook was to limit the API to test what we actually need in there. Since useQuery has been around a long time, I realize some of those options might be outdated and/or they don't make as much sense these days, so I didn't want to just add stuff that might not make sense. Limiting the scope of useSuspenseQuery is a good feedback mechanism to ensure that additional options have a specific use case and a reason to exist.

To be honest, I've been working on a demo on the side to get a feel for useSuspenseQuery and have started feeling like this option might be necessary as well. Glad to hear you also have a use case for it. To me the trickiest thing here is just making sure the TypeScript return type is correct. One benefit of useSuspenseQuery is that we can type the return value without undefined (i.e. TData vs TData | undefined) and this becomes a bit tricker with options like skip. I'll try and think through the best course of action for this.

Thanks for the feedback!

[laverdet]

Would you be able to give me some pseudo code to sketch out what you mean here?

// Prime the cache for the upcoming component
useBackgroundQuery(query1);
useBackgroundQuery(query2);

// [...] a bunch of other code, maybe even in another component

const data = useSuspenseQuery(query1);
// we don't need this anymore because the product specification changed
// const otherData = useSuspenseQuery(query2);

I've never used Relay but I understand a big part of the framework is a compiler which lifts query fragments out of child components to combine them into dispatchable queries in a top component. This ensures that each and every field in a query is used by some component. I don't know how usePreloadedQuery fits into this, but based on the light amount of documentation I've read it seems like overfetching was enough of a problem at Facebook that they wrote a whole compiler to avoid it. useBackgroundQuery seems like it's walking toward the same hazard.

To me the trickiest thing here is just making sure the TypeScript return type is correct

You can do it with conditional types:

type MaybeTypedDocumentNode<Data, Variables> = TypedDocumentNode<Data, Variables> | null;
type ResultForMaybeTypedDocumentNode<Type> =
	Type extends TypedDocumentNode<infer Data, infer Variables>
		? QueryResult<Data, Variables>
		: null;

export function useSuspenseQuery<Query extends MaybeTypedDocumentNode<Data, Variables>, Data, Variables>(
	query: Query,
	unstableVariables: Variables,
	options?: QueryOptions,
): ResultForMaybeTypedDocumentNode<Query> {

If you wanted to maintain the skip parameter instead of passing a null document you could also do that by making skip a generic extends boolean type in QueryHookOptions. Then you'd take a union with true extends Skip ? null : never in the result type. I personally found null documents easier to implement because the whole implement more or less shakes out naturally with proper usage of useEffect.