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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,8 @@

### vNEXT

- 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

- `apollo-server-fastify` is now on Apollo Server and lives within the `apollo-server` repository. This is being introduced in a _patch_ version, however it's a _major_ version bump from the last time `apollo-server-fastify` was published under `1.0.2`. [PR #1971](https://github.com/apollostack/apollo-server/pull/1971)
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-core/src/types.ts
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
@@ -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
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
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