Skip to content
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

Demonstrate Apollo-Server context usage with-apollo-xxx examples #10413

Closed
ivan-kleshnin opened this issue Feb 4, 2020 · 10 comments · Fixed by #42771
Closed

Demonstrate Apollo-Server context usage with-apollo-xxx examples #10413

ivan-kleshnin opened this issue Feb 4, 2020 · 10 comments · Fixed by #42771
Labels
examples Issue/PR related to examples good first issue Easy to fix issues, good for newcomers

Comments

@ivan-kleshnin
Copy link
Contributor

ivan-kleshnin commented Feb 4, 2020

Feature request

Current with-apollo- examples do not demonstrate how to use Apollo-Server with context resolver. It doesn't work out of the box and it's not at all straightforward for Apollo newbies (like myself) how to enable it.

Note: this is a request for help/consulting for Apollo experts.

1. createIsomorphLink

createIsomorphLink implementations recommended by Apollo Team imply the use of
SchemaLink on server and HttpLink on client. SchemaLink is said to be more performant
because it avoids HTTP layer alltogether:

function createIsomorphLink(ctx) {
  if (typeof window == 'undefined') {
    // !!! Server side
    let {SchemaLink} = require('apollo-link-schema')
    let {schema, context} = require('./schema')
    return new SchemaLink({schema, context : ctx}) 
  } else {
    // !!! Browser side
    let {HttpLink} = require('apollo-link-http')
    return new HttpLink({ 
      uri: '/api/graphql',
      credentials: 'same-origin',
    })
  }
}

2. SchemaLink

SchemaLink does not initiate an HTTP request. The side effect is that Apollo-Server context function is never called. This function is crucial as it usually fetches or polyfills all common resolver's data including current user/visitor, etc. Current with-apollo- examples demonstrate how to read user data in local (leaf) resolvers which is not production-like.

Luckily, SchemaLink accepts a context argument where we can provide the same context resolver as to the new ApolloServer({context: ...}).

3. Two calls

Now the tricky part is that createIsomorphLink is called twice. Once with {req, res} context data (coming from getInitialProps) and another – with no context (coming from these:

 const WithApollo = ({ apolloClient, apolloState, ...pageProps }) => {
    const client = apolloClient || initApolloClient(undefined, apolloState) // !!!

lines).

context function of Apollo-Server must not be called with empty context as it, in most cases, reads ctx.req.

The second invocation of Apollo-Client will reuse cache so resolvers, including context won't be called. It seems to me that an update like:

function createIsomorphLink(ctx) {
  ...
-  let {schema} = require('./schema') 
-  return new SchemaLink({schema, context: ctx)
+  let {schema, context} = require('./schema') 
+  return new SchemaLink({schema, context : isEmpty(ctx) ? context(ctx) : ctx})
}

should be enough to enable Server's context usage. Given that apollo/schema.js exports standalone context function of course.

I can make a PR but, before that, it would be great to get a feedback from someone more experienced with Apollo and NextJS.

Are my reasoning correct? Is my proposal valid?

Another option it to always use HttpLink like it's done in https://github.com/adamsoffer/next-apollo and https://github.com/lfades/next-with-apollo For some reason the authors of both packages decided to not rely on SchemaLink...

@ivan-kleshnin
Copy link
Contributor Author

ivan-kleshnin commented Feb 4, 2020

cc @adamsoffer @lfades I believe you guys can contribute something useful to this discussion.
Why did you chose to avoid SchemaLink in particular?

@Timer Timer added good first issue Easy to fix issues, good for newcomers help wanted labels Feb 4, 2020
@lfades
Copy link
Member

lfades commented Feb 4, 2020

I chose to review and approve PRs to update the examples, I've been disconnected from Apollo for a while now, and because it has been changing so much, the community is the one deciding the state of the examples.

If the approach you're mentioning is better and the proof is there, you're welcome to create a PR with the changes.

@ghost
Copy link

ghost commented Feb 5, 2020

I was just finding this same issue with the apollo Auth example.

Context will likely want more than just the standard ctx - for example, id like to check JWT authentication token from the cookie in the context function and apply user ID to the context or throw an auth error, so i would recommend always using a context function rather than only when empty, the cache sounds correct though.

return new SchemaLink({schema, context : getContext(ctx)})

@ivan-kleshnin
Copy link
Contributor Author

ivan-kleshnin commented Feb 5, 2020

@glenarama your code won't work by the reasons I described above.

I'm pretty sure you didn't completely understand my post. Please read Two calls section thoroughly.

  1. You can't provide meaningful context to SchemaLink in all scenarios.
  2. You can't rely on fake (empty) ctx in context resolver.

for example, id like to check JWT authentication token from the cookie in the context function and apply user ID to the context or throw an auth error

This is exactly the reason I propose tne above.
But you have to play with the code a bit to get what I'm saying. It's not as trivial as it may seem.

@ghost
Copy link

ghost commented Feb 6, 2020

I should have noted that the getContext returns an empty object if no req is passed, this works fine for me.
Context isn't needed at all on the second pass - certainly in my case your second point is true: You can't rely on fake (empty) ctx in context resolver.
Im passing null on the second pass and it works great.
Another way (clearer) of writing what i had above is:

return new SchemaLink({schema, context : isEmpty(ctx) ? null : getContext(ctx)})

@ivan-kleshnin
Copy link
Contributor Author

In that case we're on the same page!
null vs {} as well as the way to check for it are implementation details IMO.

@HaNdTriX
Copy link
Contributor

HaNdTriX commented Feb 24, 2020

@ivan-kleshnin I fixed your issue (3. Two calls) in the following PR #10451

Since the example currently doesn't implement a server schema I never added SchemaLink recommendation. Just in case you want to implement this yourself please make sure that the schema won't be shipped to the client.

@eric-burel
Copy link
Contributor

eric-burel commented Mar 17, 2020

Hi, just to give feedback we use SchemaLink in Vulcan without much trouble as far as I know. We define different clients for SSR and client-side rendering, literally in different files.

Having worked with isomorphic code and SSR for a while, we now tend to consider calls like ssr: Boolean(ctx) to be anti-pattern, except if you really can't avoid it or it's done in a "clean" and limited manner like in Next pages.

This is because server-side code tend to diverge slowly from client-side code, and it becomes unmanageable in an app that is multiple years old (unclear bundling process, require() everywhere). Even if it look alike in the beginning you quickly hit issues like needing different link depending on the environment. The first time you wonder "am I server-side?" in your code is probably the good time to split your file in 2 for client and server. (edit: or most of the time 3 files, one for common code, and 2 smaller files with specific code for each env)

An important step is to unify context between REST calls (here the API routes), graphQL calls server side, and graphQL calls client-side. For some reason using a context in queries is not that common in traditional Node app. As far as I understand that's the point of this issue.

@eric-burel
Copy link
Contributor

Btw I do not agree with the "good-first-issue" tag, this is trickier than it seems and raises questions about state-of-the-art patterns like isomorphic code. There is no clear consensus about how to manage such issues.

@Timer Timer removed the help wanted label Jun 3, 2020
@balazsorban44 balazsorban44 added the examples Issue/PR related to examples label Jan 12, 2022
@kodiakhq kodiakhq bot closed this as completed in #42771 Nov 14, 2022
kodiakhq bot pushed a commit that referenced this issue Nov 14, 2022
…ons/next (#42771)

Closes #42769

## Description 

This PR address #42769 by updating the `api-routes-apollo-server`, `api-routes-apollo-server-and-client` and `api-routes-apollo-server-and-client-auth` examples to use Apollo Server 4 and [@as-integrations/next](https://github.com/apollo-server-integrations/apollo-server-integration-next), which is the Apollo Server Next integration package. The PR also updates the three examples to use Typescript. The functionality of the examples is the same.


## Documentation / Examples
- [X] Make sure the linting passes by running `pnpm build && pnpm lint`
- [X] The "examples guidelines" are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md)


closes #33545
closes #30082
closes #21984
closes #10413
@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue/PR related to examples good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants