-
Notifications
You must be signed in to change notification settings - Fork 2k
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
apollo-server-lambda: outsource Lambda expertise #5211
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glasser
force-pushed
the
glasser/lambda-vendia
branch
3 times, most recently
from
May 13, 2021 23:49
3a3d498
to
7d4854a
Compare
Before this change, apollo-server-lambda contained a lot of code that tried to understand the formats of Lambda event (request) and result (response) formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion 1.0 and 2.0 as well as ALB. But understanding the intricacies of the various Lambda message formats isn't really what the Apollo Server project is about. A pretty large fraction of all maintenance on Apollo Server in 2021 has gone into tweaking details of the Lambda event parsing and making the Lambda handler support all AS features without the help of a library supporting composable middleware. This PR (targeted for AS3) throws away the laboriously constructed bespoke Lambda parsing and faux-middleware implementation and replaces it with two packages that solve these problems for us: `@vendia/serverless-express` which understands a variety of Lambda input and output formats and converts them to Express format, and `express`, the most popular Node library for defining HTTP server behavior. (Note that `@vendia/serverless-express` is not related to the `serverless` CLI/framework.) Now `apollo-server-lambda` is just a convenience wrapper around `apollo-server-express`. It has to deal with the difference in startup logic between serverless and non-serverless environments, but it doesn't have to reimplement all of the Lambda and HTTP logic from scratch. As an added advantage, you can now optionally provide your own express app to `apollo-server-lambda`. Previously, `apollo-server-lambda` gave no real way to customize any of its behavior past the particular options we define, because Lambda doesn't have a built-in middleware framework. Since we are removing some built-in features like `graphql-upload` integration in AS3, it's important that we continue a way to add custom behavior to your Lambda server. Letting you define that custom behavior with a standard Express app seems reasonable. We recognize that Lambda users generally care strongly about bundle size, so adding two new dependencies may seem problematic. That said, we don't currently have a principled way of evaluating Lambda bundle sizes when we make choices in this project, and compared to other dependencies of Apollo Server, these new dependencies are not very large. For now, the improvement in maintainability and flexibility seems worth the bundle size increase. If `apollo-server-lambda` users want to help out with a new project of focusing on Lambda bundle size optimization, we can work together to define benchmarks based on realistic build/bundler conditions, and find other ways to reduce bundle size (eg, there is a fair amount of low hanging fruit inside `apollo-reporting-protobuf`). Fix inconsistency in the content-type returned from health checks (the old Lambda used `application/json` instead of `application/health+json` like most other integrations). This new version passes all of the existing integration tests (plus the `testApolloServer` suite from `apollo-server-integration-testsuite/src/ApolloServer.ts` which wasn't being run previously!) essentially out of the box. (I fleshed out the "mock server" implementation which converts from Node http requests to Lambda events a bit more, and changed, but no "core" code or test changes were needed other than fixing the health check `content-type`.) Fixes #5078. Fixes #4951 (because that API is just "Express").
glasser
force-pushed
the
glasser/lambda-vendia
branch
from
May 21, 2021 05:32
7d4854a
to
6a61960
Compare
@abernix post-merge review here may be interesting. |
glasser
added a commit
that referenced
this pull request
May 21, 2021
…tions In AS2, the `apollo-server-lambda` context function receives `event` and `context` options. PR #5211 moved these under a `lambda` option and added `express: { req, res }`. While this choice added clarity, it also is just an unnecessary hoop for upgraders to jump through. This is `apollo-server-lambda` so it's not unreasonable for the top-level options to be about Lambda. So this moves `event` and `context` back to the top.
Note that #5229 moves event and context back to the top level of the context args. |
glasser
added a commit
that referenced
this pull request
May 21, 2021
…tions (#5229) In AS2, the `apollo-server-lambda` context function receives `event` and `context` options. PR #5211 moved these under a `lambda` option and added `express: { req, res }`. While this choice added clarity, it also is just an unnecessary hoop for upgraders to jump through. This is `apollo-server-lambda` so it's not unreasonable for the top-level options to be about Lambda. So this moves `event` and `context` back to the top.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, apollo-server-lambda contained a lot of code that tried to
understand the formats of Lambda event (request) and result (response)
formats. It tried to work with APIGatewayProxy messages of payloadFormatVersion
1.0 and 2.0 as well as ALB.
But understanding the intricacies of the various Lambda message formats isn't
really what the Apollo Server project is about. A pretty large fraction of all
maintenance on Apollo Server in 2021 has gone into tweaking details of the
Lambda event parsing and making the Lambda handler support all AS features
without the help of a library supporting composable middleware.
This PR (targeted for AS3) throws away the laboriously constructed bespoke
Lambda parsing and faux-middleware implementation and replaces it with two
packages that solve these problems for us:
@vendia/serverless-express
whichunderstands a variety of Lambda input and output formats and converts them to
Express format, and
express
, the most popular Node library for defining HTTPserver behavior. (Note that
@vendia/serverless-express
is not related to theserverless
CLI/framework.)Now
apollo-server-lambda
is just a convenience wrapper aroundapollo-server-express
. It has to deal with the difference in startup logicbetween serverless and non-serverless environments, but it doesn't have to
reimplement all of the Lambda and HTTP logic from scratch.
As an added advantage, you can now optionally provide your own express app to
apollo-server-lambda
. Previously,apollo-server-lambda
gave no real way tocustomize any of its behavior past the particular options we define, because
Lambda doesn't have a built-in middleware framework. Since we are removing some
built-in features like
graphql-upload
integration in AS3, it's important thatwe continue a way to add custom behavior to your Lambda server. Letting you
define that custom behavior with a standard Express app seems reasonable.
We recognize that Lambda users generally care strongly about bundle size, so
adding two new dependencies may seem problematic. That said, we don't currently
have a principled way of evaluating Lambda bundle sizes when we make choices in
this project, and compared to other dependencies of Apollo Server, these new
dependencies are not very large. For now, the improvement in maintainability and
flexibility seems worth the bundle size increase. If
apollo-server-lambda
users want to help out with a new project of focusing on Lambda bundle size
optimization, we can work together to define benchmarks based on realistic
build/bundler conditions, and find other ways to reduce bundle size (eg, there
is a fair amount of low hanging fruit inside
apollo-reporting-protobuf
).Fix inconsistency in the content-type returned from health checks (the old
Lambda used
application/json
instead ofapplication/health+json
like mostother integrations).
This new version passes all of the existing integration tests (plus the
testApolloServer
suite fromapollo-server-integration-testsuite/src/ApolloServer.ts
which wasn't being runpreviously!) essentially out of the box. (I fleshed out the "mock server"
implementation which converts from Node http requests to Lambda events a bit
more, and changed, but no "core" code or test changes were needed other than
fixing the health check
content-type
.)Fixes #5078. Fixes #4951 (because that API is just "Express").
TODO: