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

Provide more accurate types for apollo-server-expresse's ContextFunction #2330

Merged
merged 5 commits into from Feb 19, 2019

Conversation

cheapsteak
Copy link
Member

This was an issue brought up in #1593

This PR should provide more accurate typing information for apollo-server and apollo-server-express

  • ContextFunction can return a new context object synchronously
  • context for apollo-server and apollo-server-express will contain req and res

TODO:

  • Update CHANGELOG.md with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

This looks great!

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Me: "Looks great!"
Also me: Accidentally clicks "Request changes".

@abernix abernix merged commit 6308996 into master Feb 19, 2019
@abernix abernix deleted the chang/context-typing branch February 19, 2019 18:00
@abernix
Copy link
Member

abernix commented Feb 20, 2019

This has been included in 2.4.3. I plan on promoting it to the latest tag tomorrow, but you can try this right now using the next tag or by directly installing 2.4.3.

@nklayman
Copy link

Could you export this as well so it can be used when defining a context type (to be used with strongly-typed resolvers)? Right now I have to do this:

import { User } from 'prisma-client'
import { IncomingMessage } from 'http'

export interface MyContext {
  user: User
  request: {
    res: IncomingMessage
    req: IncomingMessage
  }
}

@cheapsteak
Copy link
Member Author

@nklayman great point
Would this PR by KATT solve that issue? #2352

@nklayman
Copy link

Yep, this line should do it.

@cheapsteak
Copy link
Member Author

@nklayman released as 2.4.5
Thanks for surfacing the issue!

}

export interface ApolloServerExpressConfig extends Config {
cors?: CorsOptions | boolean;
Copy link
Member

Choose a reason for hiding this comment

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

@cheapsteak I think bringing this cors property into the ApolloServerExpress config may actually go against the existing patterns for users of apollo-server-express, which I believe has typically asked for cors to be set as part of applyMiddleware.

For example, as #1882 points out, there's a typing issue on the ApolloServer exported from the standalone apollo-server package (which does not require or have applyMiddleware since it's "standalone") that seems to omit the cors property.

While the integrations pass their cors settings via applyMiddleware, the standalone doesn't have applyMiddleware, so it was necessary to provide desired cors settings via the constructor.

Currently, this new typing seems to almost encourage apollo-server-express users to set their cors in the constructor as well, even though all the other integrations require cors to be set in the applyMiddleware. In fact, I don't believe that setting cors via apollo-server-express's ApolloServer constructor would actually work since it's only in the apollo-server implementation that we set this.cors = config && config.cors;.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point, appreciate the thorough breakdown
Opened a PR to address this - #2373

abernix pushed a commit that referenced this pull request Feb 25, 2019
…onstructor (#2373)

* Fix: Remove incorrect cors option type from apollo-server-express's constructor

#2330 (comment)

* update changelog

* Update CHANGELOG.md
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 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

3 participants