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

feat(lambda): support async handler functionality #2909

Closed
wants to merge 2 commits into from

Conversation

dsanders11
Copy link

@dsanders11 dsanders11 commented Jun 24, 2019

PR allows using the Lambda handler created by ApolloServer.createHandler in either async (preferred) or callback mode. The current callback implementation has issues with the Node 10 runtime (see #2705) and this PR does not attempt to fix that. Instead it sidesteps it by allowing usage without a callback, which works in Node 10.

To summarize what this means (Node 6 is aging out this month, so I won't include it since this is would be in a future release):

Version Callback Async
Node 8.10
Node 10.x ⛔️

Due to the issues the callback version has with the Node 10 Lambda runtime, it should be deprecated and the docs should be changed to use the async method.

DISCLAIMER: This PR is not heavily tested. I banged it out real quick after running into #2705 and did quick sanity checks to ensure that basic usage worked in both Node 10.x and Node 8.10. It wouldn't shock me if the automated tests fail for this PR, and it probably needs some test coverage.

@dsanders11
Copy link
Author

@abernix, could you take a look at this when you get a chance? Thanks.

@JacksonKearl
Copy link

Is this still needed with the closing of CodeGenieApp/serverless-express#234?

@dsanders11
Copy link
Author

@JacksonKearl, if Amazon fixed their Node 10.x runtime, then the one doesn't work case in the above matrix would work now, which is nice.

I still think this PR should be merged. Async is a cleaner way to implement the handler than callbacks when you need to modify the result, like adding headers. The current idiom is to use a callbackFilter (see removed code in this PR at line https://github.com/apollographql/apollo-server/pull/2909/files#diff-16d4d9e1275e5b29399d54c22f79316fL187), which is not as clean as using an async handler.

Most of this PR is just refactoring the existing code to support both paradigms, it shouldn't add much of a maintenance burden.

@dsanders11
Copy link
Author

@JacksonKearl, @abernix, can someone look at this further? It's a change worth taking, and it makes the code cleaner to follow by simplifying the async logic.

I've also got another patch (adding file upload) which I built on top of this one (6337e47). That would close out #1739, which I know a lot of people would like to see fixed.

@abernix abernix closed this Jun 24, 2020
@abernix abernix reopened this Jun 25, 2020
@abernix abernix changed the base branch from master to main June 25, 2020 09:04
@glasser
Copy link
Member

glasser commented Apr 1, 2021

Hi, I missed this PR but implemented something similar in #5004.

@glasser glasser closed this Apr 1, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants