-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(do not merge) schemalink in app-dir-experiments #36
base: main
Are you sure you want to change the base?
Conversation
this is quite interesting, I think I can change the demo to use this, maybe next week if I have time 😊 |
If I remember correctly there are 3 environments where data may be fetched (correct me if I'm missing anything):
Afaik Let's say a Subscription is not a common use case, the one I'm worried about is the mutation, have yet to see an example of a mutation outside CSR, and an app without mutations is basically a static website... |
Went looking a bit more and SSR is basically the same as CSR aside from the cache rehydration after the first render, which is why we have a condition to check if running on server we use So the actual outlier here would be RSC, dunno what a mutation would look like in this case... |
import { | ||
ApolloNextAppProvider, | ||
NextSSRInMemoryCache, | ||
SSRMultipartLink, | ||
} from "@apollo/experimental-nextjs-app-support/ssr"; | ||
import { setVerbosity } from "ts-invariant"; | ||
import { schema } from "../api/graphql/route"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about how importing schema
here could expose credentials, schema
with resolvers would need access to the DB driver, which in turn would need some credentials, we have a check for isServer
so SchemaLink
is only used in the server side, but the schema
is imported for both, meaning the bundle might end up with the driver setup in the client, and depending on how it's bundled it would have the credentials embedded somewhere, do you think I'm making a correct assumption here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my experiences with the Nextjs bundler, I would assume that the typeof window !== "undefined"
check would remove SchemaLink
as well as schema
from the client bundle shipped to the browser, but I'd be grateful if you could validate that further :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't validate, my setup uses Neo4j GraphQL schema, which is generated async, creating the wrapper with the schema makes it harder since the makeClient
function would need to be async, and Next.js complains about functional components that can't be async, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am dumb, dunno why I didn't think about this before but to solve my issue of schema generator being async, I could just wrap makeClient
into an async factory, declare <ApolloWrapper>
component as async and inside the component, await the factory to get the makeClient
function to pass it to ApolloNextAppProvider
's makeClient
prop in a synchronous way, can't believe it took me this long to think about this solution...
@luchillo17 mutations and queries should work exactly the same - I'm not 100% sure if the SchemaLink supports subscriptions, but generally I don't think that subscriptions should be used server-side, as that would mean that the server would just have to keep streaming forever (which is something React cannot do - it just does one first render pass). |
My guess at this point is that |
@@ -29,9 +26,14 @@ function makeClient() { | |||
new SSRMultipartLink({ | |||
stripDefer: true, | |||
}), | |||
httpLink, | |||
new SchemaLink({ | |||
schema, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we need to create a context
here with req
to determine user's authentication state in graphql resolvers?
@patrick91 This came up in #31 and could be an elegant solution to have the GraphQL api in a Route Handler file, while using it from the server (and also having it accessible during build).
I don't really want to add it to the
app-dir-experiments
example, but it might be worth adding that to thepolls
demo - what do you think?