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
352 stars 25 forks source link

[Bug] SchemaLink and Async schema duplicated GraphQL instance #217

Open luchillo17 opened 2 months ago

luchillo17 commented 2 months ago

Based on #188 having a Schema that is created async will give us lots of problems trying to use SchemaLink, the current state is that we can avoid using an absolute URL for the HttpLink if we use SchemaLink on the SSR side, however, if the Schema is generated async we need to handle the promise before it even lands in the ApolloNextAppProvider's makeClient factory, which doesn't accept async factories.

The following was recommended as a way to handle the promise properly, it works and the site renders properly, but the SSR side gives an error about multiple graphql instances and the render takes longer than using HttpLink with absolute URL:

As for async client components, you might want to use use instead, but it will require a split into two components and a suspense boundary to prevent your clientFactory from being called all over again:

function ParentWrapper({children}){
  const [promise] = useState(clientFactory)
  return <Suspense><ChildWrapper promise={promise}>{children}</ChildWrapper></Suspense>
}
function ChildWrapper({promise, children}){
  const makeClient = use(promise)
  return <ApolloNextAppProvider makeClient={makeClient}>{children}</ApolloNextAppProvider>
}

We can't do that on a library level since it would wrap all our user's applications into an additional suspense boundary and that changes general UX, so it the developer needs to be very aware of that.

Tried the use and useState thing, it worked but suddenly got this kind of issue, I scanned the lock file, it should only exist a single version of graphql in my app, maybe it is creating multiple instances?

image

Originally posted by @luchillo17 in https://github.com/apollographql/apollo-client-nextjs/issues/188#issuecomment-1962179974

luchillo17 commented 2 months ago

Reproduction:

My thoughts: First, I did try multiple stuff, checked the lock file to make sure only 1 version of graphql was installed, and even tried overrides to force all libraries that depend on graphql to use the same version regardless of the dependency listed (pnpm.overrides which is the equivalent of npm.resolutions prop), same issue regardless, so no, it is not an issue with the dependency version.

I think it might have to do with the way I'm using the async import to get my neo4j-driver only in the server, were I to convert this into a normal import it would break the server completely (couldn't even start it) because some of the code relies on NodeJS specific APIs, maybe the fact that my ApolloSSRWrapper gets run once per request might have something to do, or maybe my schema null check is not working as I expected to share the schema between all instances of the wrapper... image

phryneas commented 2 months ago

I'm gonna guess that you're running into a dual package hazard with the graphql module here.

I imagine your code is transpiled before being executed, so everything is a require, except for that one stray import.

The only thing I can imagine right now to get around that dynamic import would be to wrap that code into a new package that exposes different contents for different environments, based on a conditional exports field.

You can see a simple example of that here in rehackt - but you'd need to adjust it a bit:

{
  "exports": {
    ".": {
      "types": "./index.d.ts",
      "browser": "./empty.js"
      "default": "./actualCode.js"
    },
    "./package.json": "./package.json"
  },
}

That way, the browser build would always pick this up as an empty file.

I'm really sorry for all of these complications, but that's the sorry state the complexity of the React bundling experience puts us in right now.

luchillo17 commented 2 months ago

At this point given the requirement of the schema being generated async and this issue, RSC and Server Actions start to look more appealing, otherwise, I would have to stick to HttpLink, then again saving the first request for SSR might not be worth the effort, especially if the API and the UI are served from the same host.

phryneas commented 2 months ago

I'd love to have a better solution for you here - you are pretty much experiencing every pain the ecosystem has to offer :(

That said, are you 100% sure that you need to dynamically import neo4j-driver? I imagine you could move that one or two files away and it would be tree-shaken completely in the browser if you sprinkle enough typeof window === "undefined" in there 🤔

luchillo17 commented 2 months ago

Well, the typings for the driver don't have an option that doesn't return a Promise (I might have to check the src and see if there's an internal sync option), meanwhile, I am wondering if trying to activate top-level await and maybe making the output target of TS be ES2022 or NodeNext or some other config like that if it could simplify my code and make it easier to export the Schema instead of wrapping in async functions as I do now...