Add suspenseCache as a lazy hidden property on ApolloClient

[phryneas]

This is just an experiment I had in mind and wanted your feedback on @jerelmiller and @alessbell .

This removes SuspenseCache from the Provider options, and instead creates a SuspenseCache the first time it is requested by useSuspenseCache. It will be saved as a property on the ApolloClient instance, so those two instances will always be tied together (probably the biggest drawback I could see, but at the same time I would not expect people to decouple them).

Configuration options can be passed in as part of the ApolloClient configuration:

new ApolloClient({
  react: {
    suspenseCacheOptions: {
      autoDisposeTimeoutMs: 60_000
    }
  }
})

Note that the ApolloClient itself has no knowledge of any of that and is still pure "non-React".

WDYT?

(I'm perfectly fine to trash this, I just wanted to give it a quick try :) )

[changeset-bot]

🦋 Changeset detected

Latest commit: 86a0ccd

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

[github-actions]

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	37.08 KB (+0.53% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	43.67 KB (+0.49% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	42.27 KB (+0.47% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	32.4 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.21 KB (-1.67% 🔽)
import { ApolloProvider } from "dist/react/index.js" (production)	1.19 KB (-1.77% 🔽)
import { useQuery } from "dist/react/index.js"	4.25 KB (+0.05% 🔺)
import { useQuery } from "dist/react/index.js" (production)	4.08 KB (+0.05% 🔺)
import { useLazyQuery } from "dist/react/index.js"	4.57 KB (+0.03% 🔺)
import { useLazyQuery } from "dist/react/index.js" (production)	4.39 KB (0%)
import { useMutation } from "dist/react/index.js"	2.49 KB (+0.04% 🔺)
import { useMutation } from "dist/react/index.js" (production)	2.47 KB (+0.08% 🔺)
import { useSubscription } from "dist/react/index.js"	2.23 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.19 KB (+0.05% 🔺)
import { useSuspenseQuery } from "dist/react/index.js"	4.69 KB (+32.07% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production)	4.12 KB (+38.48% 🔺)
import { useBackgroundQuery } from "dist/react/index.js"	4.17 KB (+7.75% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.6 KB (+8.84% 🔺)
import { useReadQuery } from "dist/react/index.js"	2.66 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	2.63 KB (+0.08% 🔺)
import { useFragment } from "dist/react/index.js"	2.04 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	1.99 KB (0%)

[jerelmiller]

My thoughts:

What I like:

• It "just works" more out-of-the-box than it does today. Suspense is more transparent to the user since the suspense cache is created for the developer under-the-hood.
• It still keeps the React bits separate from core, which was the original goal of the suspenseCache option in ApolloProvider

What I don't like:

• We really only need a single suspense cache, so no reason we need to create a distinct one per-client. The client is part of the cache key, so swapping clients in the hooks will work properly with a single suspense cache. If we make the cache a per-client cache, we can just remove client as part of the cache key.
• I prefer the style of defining the suspense cache as new SuspenseCache(options) because its very obvious where these options are used.
• This provides another "options" type in the ApolloClient instance. We already have defaultOptions in there. I'd prefer to add a new "namespace" in defaultOptions if we went this route:

new ApolloClient({
  defaultOptions: {
    suspenseCache: {
      autoDisposeTimeoutMs: xxx
    }
  }
});

Nitpicking on my solution here: Devs might not know what a "suspense cache" is when defining via defaultOptions since the dev doesn't have any other tie to it. Nothing that can't be solved with documentation, but there is slightly more indirection. This is also a highly-advanced option that will seldom be used, so at the end of the day, probably not a huge issue.

---

I guess the question would be: do we feel this is a big enough improvement over the existing way to setup that we'd be ok making this "breaking change"? While I haven't heard complaints over the current style of creating the suspense cache, that doesn't mean others necessarily like it.

I go back and forth and could go either way.

I think if we can find a way to use new SuspenseCache(options) as the "override" and find a good way to lazy instantiate it when its not, that would take care of all of my "what I don't like" gripes. To reiterate what I mean by this:

Standard usage: no need to instantiate a suspense cache which provides the win in this PR:

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

<ApolloProvider client={client} />

// ...
useSuspenseQuery() // Works!

Overriding the suspense cache to provide options. This avoids the indirection and makes it obvious how/where those options are used.

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

// instantiate and pass the instance to use this suspense cache
<ApolloProvider client={client} suspenseCache={new SuspenseCache(options)} />

My gripes are very small and nitpicky though. Nothing we can't solve with documentation. I'd be happy to find a way to make defaultOptions work if we go that route

---

Thoughts on my thoughts 😆?

[phryneas]

• We really only need a single suspense cache, so no reason we need to create a distinct one per-client. The client is part of the cache key, so swapping clients in the hooks will work properly with a single suspense cache. If we make the cache a per-client cache, we can just remove client as part of the cache key.

Yeah, we could 100% drop that. I think as it's per-client either way, this is neither a plus nor a minus in the end? For me it was only important that we get a data structure that will get disposed of if it's not used any more, so the alternative would have been a global WeakMap with the client as a key - but since we still can't 100% rely on WeakMaps being available (can we drop that assumption in 4.0?) it seemed logical to go this way instead.

* I prefer the style of defining the suspense cache as `new SuspenseCache(options)` because its very obvious where these options are used.

Can't say anything against that :)

* This provides _another_ "options" type in the `ApolloClient` instance. We already have `defaultOptions` in there. I'd prefer to add a new "namespace" in `defaultOptions` if we went this route:

new ApolloClient({
  defaultOptions: {
    suspenseCache: {
      autoDisposeTimeoutMs: xxx
    }
  }
});

That sounds fine to me - I wasn't 100% sure if that would be the right place, so I went for the bucket approach, but in the end I don't care where these options live :)

Nitpicking on my solution here: Devs might not know what a "suspense cache" is when defining via defaultOptions since the dev doesn't have any other tie to it.
Nothing that can't be solved with documentation, but there is slightly more indirection. This is also a highly-advanced option that will seldom be used, so at the end of the day, probably not a huge issue.

That is actually my nitpick in the other direction: Most devs create a SuspenseCache without knowing what it is in the first place, so it's additional mental load - and they would need to read documentation on what it is either way.
This way, it's much more of an implementation detail than a concept they copy-paste from the docs, perceive negatively as additional boilerplate and hopefully forget about after that.

I guess the question would be: do we feel this is a big enough improvement over the existing way to setup that we'd be ok making this "breaking change"? While I haven't heard complaints over the current style of creating the suspense cache, that doesn't mean others necessarily like it.

We have to keep in mind that the audience currently installing the beta and the audience installing the final release later are probably a very different audience - I think if we can get rid of this additional mental load, that could be great.

Overriding the suspense cache to provide options. This avoids the indirection and makes it obvious how/where those options are used.

We could also think about adding a suspenseCacheOptions to the Provider. (Although I think I still slightly prefer the defaultOptions variant. How many cases where people use multiple Apollo Client instances and then also tweak the autoDisposeTimeoutMs can we realistically imagine?)

// instantiate and pass the instance to use this suspense cache
<ApolloProvider client={client} suspenseCacheOptions={options} />

Writing this, I just noticed an additional benefit all this would have: We could remove the SuspenseCache from the external exports, making it purely an implementation detail. Which means that if React would add more features natively that would allow us to forego the SuspenseCache altogether, we could just swap that out without anyone really noticing.

[phryneas]

I noticed that removing the client from the CacheKey didn't yield any type errors, so I cleaned this one up a bit.

[phryneas]

This will result in a TypeScript warning if toHaveSuspenseCacheEntryUsing will be used on anything other than an ApolloClient instance.

[phryneas]

In the future, there will not be any export of the SuspenseCache class any more, not even Apollo-Client-internally.
Instead, we export the getSuspenseCache (only for internal use, so it should not end up in any public entry points!) function.

[phryneas]

The tests are now completely unaware of the "implementation detail" SuspenseCache, apart from those using the toHaveSuspenseCacheEntry matcher - and those are also not aware of any class called SuspenseCache.

[jerelmiller]

🎉 I'm very happy with this change and I think our users will love this. I'm very glad we were able to making the suspense cache a transparent thing for users. Thanks for putting this together!