apollo-server-lambda: support Node 14 runtime

[adikari]

Lambda has two ways of defining a handler: as an async Promise-returning function, and as a callback-invoking function. https://docs.aws.amazon.com/lambda/latest/dg/nodejs-handler.html The async variety was introduced with Lambda's Node 8 runtime. Apparently the callback-invoking variety was removed with their Node 14 runtime (their docs don't mention this anywhere but our users have reported this: #1989 (comment)). While AWS doesn't directly support pre-Node-8 runtimes any more, it's possible some users are using a Custom Runtime that still requires the Node 6 version, and Apollo Server still technically supports Node 6. So we would like to support both an async and a callback-invoking handler.

To implement this, I rewrote the handler to be an async function and used a little maybeCallbackify function to make it work as either type of handler. (Apollo Server 3 will drop Node 6 support, at which point we should just make this package always return an async handler. Tracking in #5018.) This simplified a lot of the logic and I was able to replace various Promise and callback-y stuff with direct code. To make this easier, I inlined the graphqlLambda function (which was the main API in Apollo Server v1 but is not exported in v2). This makes it easier to avoid bugs like #3999.

Original version by @adikari.
Fixes #1989

[apollo-cla]

@adikari: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[adikari]

I had to change this to any to comply with the changes. If you have a better approach, please suggest and I will update it.

[abernix]

I didn't go super deep on this, but LGTM.

[abernix]

I've been // AS3-ing these sorts of things in the source, though I suppose we will also find Apollo Server 3 if we look. 😉

[glasser]

I've just been heavily filing tickets on https://github.com/apollographql/apollo-server/milestone/16 :) Good call re findability though.

[martijnwalraven]

As far as I can see, this looks fine.

My main comment is that I'm not sure how much we should really care about compatibility with pre-Node-8 runtimes, especially because people would only run into this when using a custom runtime on AWS. So I personally would have favored code readability and maintainability over concerns about backwards compatibility that seem mostly theoretical.

(And looking towards a larger Apollo Server redesign, I'd really like us to get to a point where we can remove all the duplicated logic between the different integrations.)

[glasser]

Yeah, I think the tricky bit of this PR is making sure the rewrite to async didn't miss anything, and maybeCallbackify was fast to write and will be easily deleted.

I suppose I could remove maybeCallbackify and add it back if we get any complaints at all, but it also seems to just work.

[glasser]

I'm going to merge this but with a note in CHANGELOG that we aren't sure if this actually helps with compatibility or if it's just a nice refactor (see #1989 (comment)). Hopefully @adikari can figure out what's going on before this would be released.

[glasser]

I've published a release with this to version 2.21.2-alpha.0. My plan is to release 2.21.2 in a day or two. If you want to test that this fix works before then, try running the prerelease yourself, and provide feedback on #5037. (I have not significantly QAed my changes in real Lambda contexts, so positive feedback on #5037 saying that the release works for your app would be great! It is intended to be a largely no-op change.)

[fishgills]

Is there updated documentation on how to use the new async way?

[glasser]

@fishgills The basic usage API is still the same: exports.handler = server.createHandler().

The main difference (other than the code in the package being easier to maintain) is that if you choose to wrap the handler in your own handler, you can call it with await instead of passing it a callback if you want. But I don't think we really suggest this sort of wrapping in our docs anyway.

FWIW I'm moving more and more towards the opinion that the use cases people have for wrapping the handler (or for using this package at all!) would be better served by us dropping this package entirely and just encouraging that people write

import serverlessExpress from '@vendia/serverless-express';
import express from 'express';
import ApolloServer from 'apollo-server-express';

const server = new ApolloServer({...});
const app = express();
server.applyMiddleware({ app });
exports.handler = serverlessExpress({ app });

so that instead of "wrapping the handler" folks would just do normal express stuff on their app.

[JustFly1984]

Which version of apollo-server-lambda will support Node 14?

[glasser]

As far as we understand, there wasn't a fix needed to support Node 14; I think perhaps an earlier version of the Node 14 runtime only supported "async" handlers but Lambda was changed so that wasn't the case?

We did refactor the Lambda code in v2.21.2 to be an async handler even though it wasn't actually required for Node 14 compatibility.

[JustFly1984]

Well, now Node 10 is out of LTS, and node 16 will became LTS over less than 6 months, and the only LTS version which works, kind of.. is Node 12, but it has no support for ESM import, as Node 14 has.

The issue for me is that I can't have Mode 14 lambdas for everything, and Node 12 lambdas for apollo graphql in the same serverless deploy.

What is actually required to support Node 14 by apollo?

[adikari]

The current version of this library supports node 14 and async handlers. I have been running in on production for a bit. What is the actual problem you are facing @JustFly1984 ? May be I can point you to right direction.