apollographql / apollo-client-nextjs

Apollo Client support for the Next.js App Router
https://www.npmjs.com/package/@apollo/experimental-nextjs-app-support
MIT License
358 stars 25 forks source link

How can I have clients for many different HTTP links, to implement draft previews? #67

Closed tremby closed 9 months ago

tremby commented 9 months ago

The site I'm working on queries for data from a CMS which supports previews for drafts and upcoming releases. When the user requests a preview, the front end is given a token, and based on this we add some query parameters to the GQL endpoint URL.

In the past I've done this by having a map of preview tokens (or null) to Apollo clients, and then in my getClient function taking in as an argument the preview token (or null), returning an existing client from the map if there's a matching one, and otherwise making a new client, adding it to the map, and returning it.

I'm not sure how to do something equivalent with registerApolloClient provided by this package. My first thought was that I could do something like this:

export const { getClient } = registerApolloClient((previewToken: PreviewToken | null) => {
  return new ApolloClient({
    link: authLink.concat(
      createHttpLink({
        uri: `my endpoint based on values from ${previewToken}`,
      })
    ),
    cache: new InMemoryCache(),
  });
});

But I'm getting

Argument of type '(previewToken: PreviewToken | null) => ApolloClient<NormalizedCacheObject>' is not assignable to parameter of type '() => ApolloClient<any>'.
  Target signature provides too few arguments. Expected 1 or more, but got 0.

Plus I don't know whether the underlying implementation would correctly keep the different clients separate.

jerelmiller commented 9 months ago

@tremby this is an interesting use case! The registerApolloClient function uses React.cache under the hood to store this value. React's API does appear to take values, so theoretically we should allow you to pass args. Feel free to submit a PR if you'd like to implement.

@phryneas do you see any problem with allowing args to getClient and forwarding that to React.cache to allow multi-client instantiation?

tremby commented 9 months ago

I actually went ahead and tried my code snippet above, and as far as I can tell it's working perfectly with no code changes.

It's just that typescript is upset with me, because it thinks I shouldn't be allowed to pass any arguments to registerApolloClient. Perhaps we can update the types somehow?

I'm not sure what the correct signature would be, however.

tremby commented 9 months ago

I could be wrong actually. Nextjs's fetch caching is caching them separately. But I don't know what's going on under the hood with Apollo; maybe it's giving the same client all the time.

jerelmiller commented 9 months ago

@tremby ah right that makes sense since we just pass makeClient directly to React.cache. You're right, this is more of a TS issue since we expect no args for that makeClient function.

To my knowledge, that React.cache uses the input values to determine whether to create a new cache entry or not. See this RFC, specifically the code block under that section which has the following comment:

// A cached function returns the same response for a given set of inputs —
// id, in this example. The `cache` proposal will also have a mechanism to
// invalidate the cache, and to scope it to a particular part of the UI.
const fetchNote = cache(async (id) => {
  const response = await fetch(`/api/notes/${id}`);
  return await response.json();
});

As far as I can tell, you should be getting separate clients for different arguments, so it should be working as expected.

tremby commented 9 months ago

Yeah, I was about to follow up: it is working correctly. Nothing extra needs to be passed through. React.cache is caching the function which already has all knowledge of what to do with the arguments. The different endpoints are being called as I expect, and I'm getting the preview data I expect back; that means I'm getting different Apollo clients returned.

So yeah, it's just a case of opening up the types, I think.

Would it be a case of making registerApolloClient a generic? Would it be something like...

export function registerApolloClient<Args extends any[] = any[]>(makeClient: (...args: Args) => ApolloClient<any>) {
  ...
}
tremby commented 9 months ago

Works for me, I'll open a PR.

tremby commented 9 months ago

For my current project I don't think I need to enable previews in areas other than those handled by RSC. But what if I did? Any thoughts on how I'd do something similar to this for ApolloWrapper?

phryneas commented 9 months ago

I've just answered in #68 and am just seeing this.

Honestly, I think you might be overcomplicating things here. You could have just one Apollo Client, with even just one HttpLink, and handle things using request context.

const previewResult = await useClient().query({
  query,
  variables,
  // you probably want to leave the results of this query out of the cache to not mix up with normal responses
  fetchPolicy: "no-cache",
  context: {
    uri: "someDifferentUri",
    headers: { authorization: `Bearer ${token}` },
  },
});

or if you really want separate caches, you can always call registerApolloClient multiple times with different arguments.

tremby commented 9 months ago

OK, thanks for your response @phryneas. It looks like I need to reassess how I'm doing things.

I didn't realize I could change the URL at query time.

I am somewhat confused about the caches -- there's the Apollo cache and then there's Next's fetch cache. Your suggested code to avoid storing in the cache may avoid storing in Apollo's cache, but it's still storing in Next's fetch cache and I'm getting cache hits on successive requests. It's not immediately clear when to do fetchPolicy: "no-cache" vs context: { fetchOptions: { cache: "no-store" } } vs both vs neither. Something in the docs about this might be helpful.

Are you saying the Apollo client -- and therefore its cache? -- only survives a single request cycle? If so, is the Apollo cache really doing much at all, if I'm only making a couple of queries per request? Surely there's no harm in letting it populate, if it's never being reused after the request is over anyway. And if it's one cache per request cycle I also don't care about preview and live data being mixed.

(If it's a different preview token per CMS edit I can let Next's fetch cache populate too, since the URLs will be different so they'll have different cache keys. But I need to verify that first.)

phryneas commented 9 months ago

I am somewhat confused about the caches -- there's the Apollo cache and then there's Next's fetch cache. Your suggested code to avoid storing in the cache may avoid storing in Apollo's cache, but it's still storing in Next's fetch cache and I'm getting cache hits on successive requests. It's not immediately clear when to do fetchPolicy: "no-cache" vs context: { fetchOptions: { cache: "no-store" } } vs both vs neither. Something in the docs about this might be helpful.

You are not the only one confused there - Vercel seems to slap a cache on top of every part of their stack that they can think of - and generally when you do things on the server with e.g. php, caches are always a late-stage opt-in optimizations most applications never need.

Personally, I'd opt out of most of these caches. They add a lot of headache, and I have real doubts on their usefulness.

Are you saying the Apollo client -- and therefore its cache? -- only survives a single request cycle?

Yes, Next.js has no concept of persistence of sessions, and every request is always isolated in itself, so you have to start with a new, empty cache every time. The Apollo InMemoryCache can still do some useful things, like allowing you to fire a big composed query in a parent component and reading cache.readFragment in child components, but on the server it is more a client than a cache.