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

Fix Lambda context function typing #5481

Merged
merged 1 commit into from Jul 15, 2021
Merged

Conversation

glasser
Copy link
Member

@glasser glasser commented Jul 14, 2021

In AS3 we made apollo-server-lambda inherit from apollo-server-express,
but didn't realize that it also inherited the more precise TS typings on
its constructor that meant that its context function would take
Express-specific parameters (rather than the default unconstrained
parameter).

This change makes it easier for any ApolloServer subclass to declare its
context function typing, and updates apollo-server-express and
apollo-server-lambda to do so. The other integrations are left with the
vaguer defaults.

Tests didn't catch this before because they didn't pass a context
function directly to the Lambda ApolloServer constructor, just via a
function with a looser signature.

Fixes #5480.

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

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

Shared some concerns offline about ProducedContext === TContext, just noting for awareness but I don't propose we do anything about threading TContext correctly in this PR. I do think there are some places it can be improved upon quite a bit!

CHANGELOG.md Outdated
@@ -9,6 +9,8 @@ The version headers in this history reflect the versions of Apollo Server itself

## vNEXT

- `apollo-server-lambda`: Fix TypeScript types for `context` function. (In 3.0.0, the TS types for the `context` function were accidentally inherited from `apollo-server-express` instead of using the correct Lambda-specific types). [PR #XXXX](https://github.com/apollographql/apollo-server/pull/XXXX)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `apollo-server-lambda`: Fix TypeScript types for `context` function. (In 3.0.0, the TS types for the `context` function were accidentally inherited from `apollo-server-express` instead of using the correct Lambda-specific types). [PR #XXXX](https://github.com/apollographql/apollo-server/pull/XXXX)
- `apollo-server-lambda`: Fix TypeScript types for `context` function. (In 3.0.0, the TS types for the `context` function were accidentally inherited from `apollo-server-express` instead of using the correct Lambda-specific types). [PR #5481](https://github.com/apollographql/apollo-server/pull/5481)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, applied directly.

@glasser glasser force-pushed the glasser/lambda-context-fix branch 2 times, most recently from 79ca410 to 758450c Compare July 15, 2021 05:36
In AS3 we made apollo-server-lambda inherit from apollo-server-express,
but didn't realize that it also inherited the more precise TS typings on
its constructor that meant that its `context` function would take
Express-specific parameters (rather than the default unconstrained
parameter).

This change makes it easier for any ApolloServer subclass to declare its
context function typing, and updates apollo-server-express and
apollo-server-lambda to do so. The other integrations are left with the
vaguer defaults.

Tests didn't catch this before because they didn't pass a `context`
function *directly* to the Lambda ApolloServer constructor, just via a
function with a looser signature.

An earlier version of this PR attempted to add the context object type
(ie TContext) to the ApolloServer generics as well. But it didn't
actually connect through to the TContext on processHTTPRequest and
trying to link those up ran into some technical issues, so I decided to
leave it out.

Fixes #5480.
@glasser glasser force-pushed the glasser/lambda-context-fix branch from 758450c to 3310405 Compare July 15, 2021 05:36
@glasser
Copy link
Member Author

glasser commented Jul 15, 2021

Rather than add a TContext generic that isn't actually connected to the one used when actually calling the context function, I ended up removing it. Trying to connect them ran into various issues (like the default value of {} for context used when no context is provided not working because it clearly isn't correct for arbitrary context types).

@glasser glasser merged commit 928f709 into main Jul 15, 2021
@glasser glasser deleted the glasser/lambda-context-fix branch July 15, 2021 05:43
@glasser
Copy link
Member Author

glasser commented Jul 16, 2021

Fix released in 3.0.1.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 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.

AS3 apollo-server-lambda context callback parameter TS types are wrong
2 participants