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

Update most Apollo Server examples to Apollo Server 3 #30082

Closed
wants to merge 1 commit into from

Conversation

glasser
Copy link

@glasser glasser commented Oct 19, 2021

Previously, most of the examples used Apollo Server 2. As a maintainer of Apollo
Server, we frequently found that users would start with one of these examples,
try to upgrade to Apollo Server 3, and get confused when it didn't work. Apollo
Server 3 has a more explicit lifecycle that requires awaiting a start method
on startup, and users often put this in the wrong place. The
api-routes-graphql package did use Apollo Server 3 and did call start, but
only awaits it when the first request comes in rather than at the top level.

Additionally, all the examples use apollo-server-micro. While this package is
technically a maintained part of the Apollo Server project, it is not as fully
featured or battle-tested as the most popular package,
apollo-server-express. For example, it doesn't have the same default CORS
behavior has the other Apollo Server framework integrations, and doesn't have a
way to drain servers gracefully on shutdown. (Future Apollo Server features may
target apollo-server-express before other integrations as well.) Because
Next.js can easily use Express middleware with just a few lines of integration
code (documented at
https://nextjs.org/docs/api-routes/api-middlewares#connectexpress-middleware-support),
Next.js users would be best served by using the most mainstream ApolloServer
implementation rather than the least maintained one.

So this PR:

  • Changes all examples from using apollo-server-micro v2 to
    apollo-server-express v3
  • Uses top level await (enabled in next.config.js) for proper startup handling
  • Upgrades other packages like graphql and @graphql-tools/schema, and
    installs them where relevant
  • Removes special CORS handling from examples/api-routes-graphql because
    apollo-server-express has better CORS defaults. (If the default is not good
    enough for this example, pass a cors option to getMiddleware instead of
    doing CORS manually. The value of the cors option is equivalent to the
    argument to the npm cors package's middleware.)

This leaves the with-apollo-neo4j-graphql example alone, as I could not get it
to work properly before or after upgrading Apollo Server.

Documentation / Examples

  • Make sure the linting passes

Copy link
Contributor

@elrumordelaluz elrumordelaluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is missing to add express into package.json as a dependency based on this section of the Migration to v3 docs:

In Apollo Server 3, these packages have peer dependencies on their corresponding framework packages. This means that you need to install a version of that package of your choice yourself in your app (though most likely that was already the case).

@glasser
Copy link
Author

glasser commented Oct 20, 2021

@elrumordelaluz I'm not a Next.js expert but I don't think that's true here. We're not actually using Express --- we're just converting an Express-style middleware into a Next.js-style handler using the pattern from the Next.js docs.

@elrumordelaluz
Copy link
Contributor

elrumordelaluz commented Oct 20, 2021

We're not actually using Express --- we're just converting an Express-style middleware into a Next.js-style handler using the pattern from the Next.js docs.

Did you tried to run one of those examples?
I replicate your changes in one of my projects setup and this warning throws:

Error: Cannot find module 'express'
Require stack:
- /…/node_modules/apollo-server-express/dist/ApolloServer.js
...

Seems to be due to this.

@glasser
Copy link
Author

glasser commented Oct 20, 2021

@elrumordelaluz Hmm, I did run them and they worked for me, but you're right that there is a runtime dependency on express (for express.Router(). Let me see why they worked for me and add the dependencies.

@glasser glasser force-pushed the glasser/update-apollo-server branch from ad31825 to 3488f39 Compare October 20, 2021 17:32
@glasser
Copy link
Author

glasser commented Oct 20, 2021

@elrumordelaluz I'm not sure why it worked for me before; maybe a difference in what npm/yarn installation we used which made mine install peer dependencies automatically? Anyway, added the dependency to all the examples.

@patryk-smc
Copy link

I have been using Apollo Server with Next.js from some time and was about to start a new project. I checked out this example and to be honest, this feels like change for the worse, compared to previous starter. The fact that is uses webpack experimental topLevelAwait, it requires express dependency, and it relies on a function that pretends to be express middleware, makes it feel really fragile. I wouldn't use it personally.

@elrumordelaluz
Copy link
Contributor

@patryk-smc could you expand a little more?

I am also using Apollo in several NextJs projects, and I feel more that the change is between Apollo 2 and 3. In case you were using Apollo 3, what is the main difference between using apollo-server-micro + micro deps and apollo-server-express and express?
The issue with the experimental could be bypassed with something like:

const apolloServer = new ApolloServer({ /* ... */ })
const startServer = apolloServer.start()

export default async function handler(req, res) {
  // ...

  await startServer
  await apolloServer.createHandler({
    path: '/api/graphql',
  })(req, res)
}

Sincerely don't know about heavyness of each apollo-server-* package and which one is more maintained than the other (all stuff discussed in the issues related), but I think the most important point is that the community mostly copy/paste examples assuming are best-practices, but not always this is true (this is also discussed there).

Hope in one of this threads/discussions we can arrive into a solution for the best composition using Apollo 3 and Next.

@tettoffensive
Copy link

I have been using Apollo Server with Next.js from some time and was about to start a new project. I checked out this example and to be honest, this feels like change for the worse, compared to previous starter. The fact that is uses webpack experimental topLevelAwait, it requires express dependency, and it relies on a function that pretends to be express middleware, makes it feel really fragile. I wouldn't use it personally.

I agree that this new version requiring topLevelAwait and express is kind of a pain and I much prefer the simplicity of using apollo-server-micro but apparently that package isn't really supposed to be used in production, and the only people really using it are those basing it off the Next.js examples.

Even if you want to stick with apollo-server-micro you'll still need topLevelAwait if you want to upgrade to v3.

@glasser
Copy link
Author

glasser commented Oct 22, 2021

The problem with the await startServer solution (assuming I understand how Next.js works properly) is that this means "if there's a startup error, then make every handler invocation fail", whereas it makes more sense to be "don't even start listening for incoming connections until the server has properly started up".

The problem with using apollo-server-micro is that it was an externally-contributed package with literally 3% of the usage of apollo-server-express that frankly we shouldn't have accepted into the project. (And as far as I can tell the only reason that this 3% number is so high is because of these Next.js examples.) It already does not support all of Apollo Server's features and we are likely to not target it for upcoming features. There is a very good chance that a future version of AS will not include it (our hope is to make the interface between Apollo Server and web frameworks into a more stable API and turn most of the integrations into community-supported rather than core-supported).

Given that (as far as I understand) none of the examples actually use micro itself at all, it doesn't do Next.js users a favor to tie them to apollo-server-micro. That package should really only be used in conjunction with micro.

FWIW, if part of the issue is that having to stick runMiddleware in every example is annoying, I'd be happy to make that easier, by doing something like publishing a tiny npm package that just exports runMiddleware, or even making apollo-server-express have a method that returns a more micro-style middleware. But it doesn't seem like runMiddleware is the sticking point here?

@patryk-smc
Copy link

Thanks for the micro background, now it all makes sense.

For me, the biggest annoyance is that it requires an experimental flag, meaning that every minor webpack update could potentially break the thing. Chances are low, but still, it's a good practice to not relay on experimental stuff in production.

The middleware is not a big deal, but it would be nice to have it as a part of apollo-server-express. It would for sure make it easier for folks to get started and not have them to think why it has to be done that way.

@glasser
Copy link
Author

glasser commented Oct 22, 2021

For me, the biggest annoyance is that it requires an experimental flag, meaning that every minor webpack update could potentially break the thing. Chances are low, but still, it's a good practice to not relay on experimental stuff in production.

Sure. But on the other hand, the idea that a server might have to do async work on startup and shouldn't start serving incoming traffic until that work succeeds seems like a pretty basic capability for a server. So if webpack were to remove this later, hopefully Next.js could find another mechanism for achieving the same goal? Or if that really isn't possible, then at that point the examples could be changed to the worse "server starts up but every request fails" semantics. (This change in Apollo Server wasn't undertaken lightly — "my server had a plugin whose startup hook failed but my server runs anyway" was a major problem in Apollo Server 2 that many users ran into.)

The middleware is not a big deal, but it would be nice to have it as a part of apollo-server-express. It would for sure make it easier for folks to get started and not have them to think why it has to be done that way.

FWIW this runMiddleware was taken directly from the Next.js docs. Maybe it would make sense for it to just be exported from next or some sort of new package like @next/express-middleware? Then the Next.js docs could just say "to use Express middleware, use import { runMiddleware } from ....

@marklawlor
Copy link

@glasser As NextJS is often deployed in serverless environments, would it be more suitable for it to use .ensureStarted()? Would this help with peoples concerns around using top-level await?

@glasser
Copy link
Author

glasser commented Oct 22, 2021

I mean, I'm not exactly sure what "serverless" means here, in a more concrete sense than "runs on platforms like Lambda". If there's an issue with AS startup (ie, start throws) then that ApolloServer object will never be able to run operations successfully. So the best approach is whatever is most likely to result in the object being re-created, which might be process failure in this case. (But the best approach is not to start a brand new server on each handler invocation, because then you're doing startup more than is necessary.)

apollo-server-lambda does do something like this (https://github.com/apollographql/apollo-server/blob/c1fd1798a79343bf5803c14d82ad2279e9449ccf/packages/apollo-server-lambda/src/ApolloServer.ts#L38-L61) but that's because the Lambda runtime really doesn't support any sort of top-level async work, whereas it appears that Next.js's runtime does support that as long as you turn on the experimental flag.

@marklawlor
Copy link

Yes I noticed that's how the lambda version is implemented. Top level await is based upon on the Node runtime being >14.8, which I believe is supported by AWS lambda.

Since Node 14 is the current LTS, I think top level await should work fine, but maybe it's worth leaving a comment about .ensureStarted() for anyone still running Node 12?

@tettoffensive
Copy link

I've tried to make the same changes in my own project as seen here to see how it would work. Unfortunately, I've gotten some errors that seem to have to do with express, but I have no idea what they mean.

warn  - ./node_modules/express/lib/view.js
Critical dependency: the request of a dependency is an expression
Unhandled Runtime Error
TypeError: Cannot read properties of undefined (reading 'prototype')

Call Stack
eval
node_modules/express/lib/response.js (42:0)
Object../node_modules/express/lib/response.js
file:///.../.next/static/chunks/pages/_app.js (3827:1)

@glasser
Copy link
Author

glasser commented Oct 25, 2021

@tettoffensive The first one looks like a webpack error. I'm not very familiar with webpack. Are you able to share your configuration in a way I could reproduce it? Do these issues appear on the actual examples from this PR for you?

Is it possible for a maintainer to approve running workflows on this so I can see if it is passing CI?

@patryk-smc
Copy link

patryk-smc commented Oct 25, 2021

FYI: I successfully transitioned to apollo-server-express based on this example, but after I wanted to check new middleware functionality in next@11.1.3-canary.88, I noticed that webpack topLevelAwait: true breaks the whole next.js app.

@glasser
Copy link
Author

glasser commented Oct 25, 2021

@patryk-smc interesting, what do you mean by "breaks the whole next.js app"? Like, it just gives an error saying topLevelAwait is not supported? Or using it doesn't work well?

@eric-burel
Copy link
Contributor

Looks like some good changes that would be helpful to our team. Stumbled upon this looking for a way to integrate graphql subscriptions into nextjs. Using this method (apollo-server-express instead of micro) could I implement subscriptions? Or am I unable to do it this way because nextjs is weird about websockets?

Our team is currently thinking about running our next applications with a custom server instead in order to implement graphql subscriptions with websockets. Any input on this topic would be greatly appreciated. We are a small team without a lot of experience.

I would advise against a custom server, you are killing Next.js performance. That's pattern is something you would have found in Meteor apps for instance.
I think it's a valid and normal pattern to run Next.js app with a "satellite" Express server (or whatever) for long running tasks such as handling websockets, or cron jobs. You'd want to focus on how to efficiently reuse your code between Next and other apps, eg via a monorepo, instead of trying to fit everything in Next.js.

@httpete
Copy link

httpete commented May 2, 2022

Hit exactly this - and your snip works perfectly with v3 and next 12. I did nothing but copy the code, npm the deps, and fire.

@maxproske
Copy link
Contributor

@glasser Any thoughts on #38315 (comment)? I'm trying to bring this PR out of limbo.

Let me know your preference, and I can either: merge all api-routes-apollo-* examples into a single example using apollo-server-express and TypeScript, or advocate to have this PR merged and personally convert them to TypeScript after.

@glasser
Copy link
Author

glasser commented Jul 8, 2022

I don't use Next.JS. My only motivation here is to stop the sea of people showing up at our repo complaining about the fact that they can't figure out how to upgrade from AS2 to AS3 in Next, and also to stop people from using the least feature-complete Apollo Server flavor (apollo-server-micro) for no reason.

For what it's worth, we're nearing the end of development on Apollo Server 4, which provides an actual stable API for building web framework integrations instead of having a set of 9 fixed integrations hardcoded in core. Additionally, it will provide a simple API for folks who really want the "serverless" experience of "I'm not allowed to do any work on startup that might possibly fail, or if I do that it should just make every single subsequent request fail instead of preventing the server from starting in the first place". So given that this has already sat open for 9 months, maybe we might as well just wait for AS4.

I can't say I'm a big fan of #38315, if for no other reason than that it still uses apollo-server-micro for no reason.

It certainly does seem reasonable to have fewer AS examples in this repo; it sure was a pain to update all of them.

@sajadghawami
Copy link

sajadghawami commented Sep 21, 2022

@apuyou just tried your apollo-server-nextjs and perfectly works - thank you!

Maybe you want to update this merge-request to include your changes? That way there should not be any external dependency and this MR can finally be merged. Thoughts?

@apuyou
Copy link
Contributor

apuyou commented Oct 4, 2022

@sajadghawami Thank you for your interest!

I think we should probably wait for Apollo Server 4 to be released before updating this. AS4 is much easier to integrate with other frameworks and avoids depending on the integration being updated on each AS release.

I have created a v4 branch that uses this integration for https://github.com/apuyou/apollo-server-nextjs (it currently works with the Alpha but not the RC).
On my side, I may do a final update for AS3 to include recent security updates, but then will focus on AS4.

@glasser
Copy link
Author

glasser commented Oct 4, 2022

@apuyou Very cool to see! I was planning to come back here and replace this PR with an AS4 one, but it's even better if actual Next users do so.

My thought would be that I'd make two examples: one that uses top-level await and server.start() (to have "startup errors crash the process" semantics), and one that doesn't use top-level await and uses server. startInBackgroundHandlingStartupErrorsByLoggingAndFailingAllRequests() (to have "startup errors mean all requests will 500" semantics). Folks can choose their example based on whether they're comfortable turning on top-level await and whether they find fail-fast startup error handling important.

@ChristianIvicevic
Copy link
Contributor

one that uses top-level await

Next.js doesn't support top-level await at all in latest versions due to SWC.

@glasser
Copy link
Author

glasser commented Oct 4, 2022

yeah, it continues to be incomprehensible to me why you'd build a server platform that doesn't allow you to do possibly-failing work at startup before you start listening for connections, but hey, what do I know, Next.js is quite popular so I must be missing something! So I guess we would just make a single example that uses server. startInBackgroundHandlingStartupErrorsByLoggingAndFailingAllRequests().

@eric-burel
Copy link
Contributor

yeah, it continues to be incomprehensible to me why you'd build a server platform that doesn't allow you to do possibly-failing work at startup before you start listening for connections

To be correct, it's a limitation of SWC. The problem is more that it's being pushed as the default solution while it's clearly limited compared to Babel, missing top-level await being the biggest drawback. I still use Babel happily to this day and will switch only when SWC is mature.

@daniel-johnsson
Copy link

Anyone stumble upon this thread looking for a solution to increase the body limit and be able to use multipart upload using graphql-upload here is the solution that made it work for me going from apollo-server-micro to apollo-server-express:

function runMiddleware(
  req: NextApiRequest,
  res: NextApiResponse<any>,
  fn: Router
) {
  return new Promise((resolve, reject) => {
    fn(req, res, (result) => {
      if (result instanceof Error) {
        return reject(result);
      }

      return resolve(result);
    });
  });
}

const apolloServer = new ApolloServer({
  schema,
  dataSources: () => dataSources,
  context: contextFunction,
  plugins: [ApolloServerPluginLandingPageGraphQLPlayground()],
});

const startServer = apolloServer.start();

async function handler(req: NextApiRequest, res: NextApiResponse<any>) {
  await startServer;

  const contentType = req.headers["content-type"];
  if (contentType && contentType.startsWith("multipart/form-data")) {
    req.body = await processRequest(req, res);
  }
  const apolloMiddleware = apolloServer.getMiddleware({
    path: "/api/graphql",
    bodyParserConfig: {
      limit: "50mb",
    },
  });

  return runMiddleware(req, res, apolloMiddleware);
}
export default handler;

export const config = {
  api: {
    bodyParser: false,
    externalResolver: true,
  },
};

@sajadghawami
Copy link

@daniel-johnsson thank you! the processRequest function is missing though :)

@daniel-johnsson
Copy link

@sajadghawami processRequest is an exported function from graphql-upload
import { processRequest } from "graphql-upload";

@ijjk ijjk force-pushed the canary branch 3 times, most recently from df8579c to 47e5ebe Compare October 25, 2022 16:15
@kodiakhq kodiakhq bot closed this in #42771 Nov 14, 2022
kodiakhq bot pushed a commit that referenced this pull request 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
@glasser
Copy link
Author

glasser commented Nov 16, 2022

The only thing better than a project merging your PR is a project merging somebody else's superior PR :) Thanks @idoob and @kodiakhq!

@EarthlingDavey
Copy link

The only thing better than a project merging your PR is a project merging somebody else's superior PR :) Thanks @idoob and @kodiakhq!

Very humble. Thanks to yourself also @glasser

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
examples Issue was opened via the examples template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.