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

SSR: Absolute URI for HttpLink handling? #188

Closed luchillo17 closed 2 months ago

luchillo17 commented 3 months ago

Is this the only way to handle the absolute URL requirement for SSR?:

const httpLink = new HttpLink({
    uri:
      url ?? isServer()
        ? `${process.env['HOST']}:${process.env['PORT']}/api/graphql`
        : '/api/graphql',
  });

It seems to me like the only other way is with ENV variables based on environment, for example in development HOST grabs localhost:4200 but in production, the hostname (either IP or DNS name) should be known by the CI pipeline to embed in the env variables, such a pain for such a simple need...

phryneas commented 2 months ago

Sorry, I missed this issue!

When your Next.js server is also your GraphQL server, you can get around a network request completely by using a Schema link.

You can see an example of that here: https://github.com/apollographql/apollo-client-nextjs/pull/36/files

luchillo17 commented 2 months ago

I'll have to test again, last time I tried Schema Link, the issue was that my schema was generated async and made it hard to pass it into the client: image The issue being that the makeClient of the ApolloNextAppProvider didn't accept async client factory: image

phryneas commented 2 months ago

Hmm, that's an interesting problem. Can you maybe work with a top-level await in some way here?

If we were to allow makeClient to be async, that would only work in SSR, but not in the browser, and it might become very confusing if that method needed two implementations.

luchillo17 commented 2 months ago

Top-level await usually gets troublesome in one way or another, I tend to avoid it, will give that a try as well.

luchillo17 commented 2 months ago

I'm dumb, ok so I found a workaround, basically what I can do is turn ApolloWrapper into an async component, call my factory with await like const makeClient = await clientFactory(graphQLURI); and in the factory clientFactory get the async schema before I pass it to the returned makeClient: image

However I still get some issues getting it to run: image

luchillo17 commented 2 months ago

I created an issue in https://github.com/apollographql/federation/issues/2942 just to try and get some feedback.

I think what happens is that even with the if to ensure the part that awaits the schema, it still tries to import the @neo4j/driver which ends up importing @apollo/federation-internals, which expects to be done in the server, not even a dynamic import could help me here since the context the makeClient runs is different than the context the factory does:

image

I believe this is where the error in @apollo/federation-internals happens: image

So I'm left with an issue here where I can't properly build the schema.

phryneas commented 2 months ago

I think the problem might be your isServer function here. If you write if (typeof window == "undefined"), the bundler can remove that when bundling for the client, but if you wrap it in a function statical analysis by the bundler won't work anymore.

That said, I don't believe async client components (and the wrapper has to be a Client component!) are a stable React feature at this point.

phryneas commented 2 months ago

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.

luchillo17 commented 2 months ago

I'll try removing the isServer function, then the use one and see if that helps my db library, though I didn't even stop to consider if the typeof window === 'undefined' part was being used to treeshake or remove for the client.

luchillo17 commented 2 months ago

Huh would you look at that, the error changed as soon as I removed the function isServer, it's complaining about promises in client components so I guess I do need the use and Suspense combo?...

Also, it does need the dynamic import() or else the console error comes back. image

luchillo17 commented 2 months ago

Hmm, would it matter if the factory is called again if I move the httpLink and NextSSRApolloClient into global variables that I check for with ??= nullish assignment operator before returning?

phryneas commented 2 months ago

Then all your SSR-rendered requests share one instance of ApolloClient, potentially leaking sensitive data between requests, which I would really not recommend. (We will start adding warnings for that soon)

luchillo17 commented 2 months ago

Ah right this is the client instance, would be a different matter with a server instance right?

phryneas commented 2 months ago

That might be a misconception: Even though they are called "Client" Components, they will still run on the server for SSR on the first request of a user session.

luchillo17 commented 2 months ago

Yes, I meant that it's supposed to be a client-level instance because of ApolloClient, I do get that the first run goes in the server, then gets hydrated in the client to continue the interaction.

And now that I think about it, wouldn't that mean the SchemaLink in this case only helps with the first render and then it has to use the HttpLink for subsequent requests?

phryneas commented 2 months ago

Yes, the SchemaLink would only be used in SSR, all later requests would use a HttpLink on the client.

All we are talking about in this issue pretty much only has relevance in that "first render on the server" - that's why I'm saying you shouldn't use a global variable for it.

luchillo17 commented 2 months ago

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

luchillo17 commented 2 months ago

Ah yes I remember now, I knew I saw it somewhere, but I already put the serverComponentsExternalPackages config in place to fix this issue when I was making the API with ApolloServer, so I can't tell why it's happening again in my case... https://github.com/neo4j/graphql/issues/4297

luchillo17 commented 2 months ago

The SSR part is broken indeed, the HTML of the SSR part (the first request that should be using the SchemaLink) only contains that error template, I've updated my branch in which I'm getting that "String!" error: https://github.com/luchillo17/graph-meister/tree/feature/schema-link-wrapper

luchillo17 commented 2 months ago

And I realized we went out of the scope of the original issue, do you want me to close this one and create a new one?

phryneas commented 2 months ago

@luchillo17 Yes please open a new issue - it's hard to keep track otherwise :)

luchillo17 commented 2 months ago

TLDR to avoid having to define an absolute URL ideally we use SchemaLink, the issue about the schema being created async is a separate issue reported separately, if we can't use SchemaLink for this or any other reason, we can build the absolute URL like in the description of this issue, grabbing the host and port from environment variables, that way we can have a different host and port in local env and in a prod env, which then we can configure in our CI/CD on deployment.

github-actions[bot] commented 2 months ago

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.