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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Feb 19, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
98595b9
typing: context function could return value synchronously
cheapsteak Feb 15, 2019
b5b494b
Create ApolloServerExpressConfig type that has req and res in context
cheapsteak Feb 15, 2019
0d0b511
Have apollo-server-express's constructor also use the express-specifi…
cheapsteak Feb 15, 2019
0587be8
Update changelog
cheapsteak Feb 15, 2019
fc5b617
Merge branch 'master' into chang/context-typing
abernix Feb 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
### vNEXT

- `apollo-server-koa`: Support OPTIONS requests [PR #2288](https://github.com/apollographql/apollo-server/pull/2288)
- Add `req` and `res` typings to the `ContextFunction` argument for apollo-server and apollo-server-express. Update `ContextFunction` return type to allow returning a value syncronously. [PR #2330](https://github.com/apollographql/apollo-server/pull/2330)

### v2.4.2

Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-core/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export { KeyValueCache } from 'apollo-server-caching';
export type Context<T = any> = T;
export type ContextFunction<T = any> = (
context: Context<T>,
) => Promise<Context<T>>;
) => Context<T> | Promise<Context<T>>;

// A plugin can return an interface that matches `ApolloServerPlugin`, or a
// factory function that returns `ApolloServerPlugin`.
Expand Down
19 changes: 18 additions & 1 deletion packages/apollo-server-express/src/ApolloServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import express from 'express';
import corsMiddleware from 'cors';
import corsMiddleware, { CorsOptions } from 'cors';
import { json, OptionsJson } from 'body-parser';
import {
renderPlaygroundPage,
Expand All @@ -11,6 +11,9 @@ import {
ApolloServerBase,
formatApolloErrors,
processFileUploads,
ContextFunction,
Context,
Config,
} from 'apollo-server-core';
import accepts from 'accepts';
import typeis from 'type-is';
Expand Down Expand Up @@ -67,7 +70,21 @@ const fileUploadMiddleware = (
}
};

interface ExpressContext {
req: express.Request;
res: express.Response;
}

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

context?: ContextFunction<ExpressContext> | Context<ExpressContext>;
}

export class ApolloServer extends ApolloServerBase {
constructor(config: ApolloServerExpressConfig) {
super(config);
}

// This translates the arguments from the middleware into graphQL options It
// provides typings for the integration specific behavior, ideally this would
// be propagated with a generic to the super class
Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-express/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export {
ApolloServer,
registerServer,
ServerRegistration,
ApolloServerExpressConfig,
} from './ApolloServer';

export { CorsOptions } from 'cors';
Expand Down
4 changes: 2 additions & 2 deletions packages/apollo-server/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import net from 'net';
import {
ApolloServer as ApolloServerBase,
CorsOptions,
ApolloServerExpressConfig,
} from 'apollo-server-express';
import { Config } from 'apollo-server-core';

export * from './exports';

Expand All @@ -27,7 +27,7 @@ export class ApolloServer extends ApolloServerBase {
private httpServer?: http.Server;
private cors?: CorsOptions | boolean;

constructor(config: Config & { cors?: CorsOptions | boolean }) {
constructor(config: ApolloServerExpressConfig) {
super(config);
this.cors = config && config.cors;
}
Expand Down