-
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
Provide more accurate types for apollo-server-expresse's ContextFunction #2330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
There was a problem hiding this 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".
This has been included in 2.4.3. I plan on promoting it to the |
Could you export this as well so it can be used when defining a import { User } from 'prisma-client'
import { IncomingMessage } from 'http'
export interface MyContext {
user: User
request: {
res: IncomingMessage
req: IncomingMessage
}
} |
Yep, this line should do it. |
@nklayman released as 2.4.5 |
} | ||
|
||
export interface ApolloServerExpressConfig extends Config { | ||
cors?: CorsOptions | boolean; |
There was a problem hiding this comment.
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;
.
There was a problem hiding this comment.
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
…onstructor (#2373) * Fix: Remove incorrect cors option type from apollo-server-express's constructor #2330 (comment) * update changelog * Update CHANGELOG.md
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 synchronouslycontext
for apollo-server and apollo-server-express will containreq
andres
TODO: