More precise types #5072
More precise types #5072
Conversation
|
@jer-sen: 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/ |
| ): ( | ||
| event: APIGatewayProxyEvent, | ||
| context: LambdaContext, | ||
| callback: APIGatewayProxyCallback | undefined, | ||
| ) => void | Promise<APIGatewayProxyResult> { |
There was a problem hiding this comment.
I'm not sure I understand what your PR accomplishes. Isn't the type of the return value given when we declare the function's own type here?
Can you explain what doesn't work about the current code and how your change improves it? I don't doubt that you've got a good idea here — I just can't figure out what it is.
There was a problem hiding this comment.
Today, the handler returned by apollo-server-lambda returns void | Promise<APIGatewayProxyResult> but we can do better. We can use multiple signatures to return either void (if a callback is provided) or a promise if no callback is provided. It's in the case the callback is not used directly by AWS but included in some other code.
There was a problem hiding this comment.
Hmm, I get that that is a useful goal, but how does your PR achieve that goal given that maybeCallbackify has an explicit return type declared?
Can you share some code that works with this change but not without this change (perhaps even as an addition to the test suite, but as a comment if that's too hard)?
Also CI says this doesn't build. Perhaps this is related to my confusion.
There was a problem hiding this comment.
You're right. There was an error, I had partially deleted the return type. Is it ok now for you and the build ?
There was a problem hiding this comment.
OK, that makes more sense. But I worry that later refactorings could break this change since it's dependent on type inference of this function's return value. Can you write some TS code in packages/apollo-server-lambda/src/__tests__/ that depends on this typing improvement? It doesn't really even have to test behavior, just the typing improvements you're going for here. (Though... I seem to recall that our test suites may be in bad shape where they are not fully type-checked, which might make this a bit of a challenge to prove. But maybe you can at least share what the code that only typechecks with this change would look like?)
There was a problem hiding this comment.
I don't know how to implement a test for that but here is some code (which looks like my app) that will raise a Typescript error:
const server = new ApolloServer({
schema: someSchema,
});
const apolloGraphqlHandler = server.createHandler();
export const handler: Handler<APIGatewayProxyEvent, APIGatewayProxyResult> = async (event, context, callback) => {
// Do something such as loading config async
const res = await apolloGraphqlHandler(event, context, undefined); // res can be undefined but will not since callback argument is undefined
// Do something such as waiting for some promises
return res; // Can not return undefined => Typescript error
};
|
AS3 is imminent and I'm probably going to just solve this in AS3 by dropping the callback-based version (tracking in #5147). |
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 Until v2.21.2 our handler was only a callback-invoking function. Starting to that version, it could be called in either way. This meant that the TypeScript types for our handler were somewhat vague (see #5072). All current Node Lambda runtimes support calling async functions; the only reason that we didn't just change directly from callback-invoking to async in v2.21.2 was to support the use case of user code that wraps our handler in their own function. But now we're doing AS3 so we can make backwards-incompatible changes, so if you're doing that, you should just make your handler async too and `await` the result of our handler. Fixes #5018.
|
I appreciate this PR but am solving the problem in the imminent Apollo Server 3 by just dropping the callback style; see #5188. |
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 Until v2.21.2 our handler was only a callback-invoking function. Starting to that version, it could be called in either way. This meant that the TypeScript types for our handler were somewhat vague (see #5072). All current Node Lambda runtimes support calling async functions; the only reason that we didn't just change directly from callback-invoking to async in v2.21.2 was to support the use case of user code that wraps our handler in their own function. But now we're doing AS3 so we can make backwards-incompatible changes, so if you're doing that, you should just make your handler async too and `await` the result of our handler. Fixes #5018.
…#5188) 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 Until v2.21.2 our handler was only a callback-invoking function. Starting to that version, it could be called in either way. This meant that the TypeScript types for our handler were somewhat vague (see #5072). All current Node Lambda runtimes support calling async functions; the only reason that we didn't just change directly from callback-invoking to async in v2.21.2 was to support the use case of user code that wraps our handler in their own function. But now we're doing AS3 so we can make backwards-incompatible changes, so if you're doing that, you should just make your handler async too and `await` the result of our handler. Fixes #5018.
With this PR
createHandlerwill return a function returning preciselyvoidor a promise depending on the type ofcallbackargument (provided orundefined).