Skip to content

Commit

Permalink
Move Disabled plugins up a level and change some context typing
Browse files Browse the repository at this point in the history
Two things got a bit mixed together here.

One is that we want the Disabled plugins to all be defined directly in
plugin/index.ts, so that loading ApolloServerPluginInlineTraceDisabled
does not spend time loading the protobuf library.

The other thing is that we started thinking a bit more carefully about
plugin context generics. We wrote some tests to make sure that you can
use an `ApolloServerPlugin<BaseContext>` (ie, a plugin that doesn't need
any particular fields on the context) with any `ApolloServer`, but not
vice versa. As part of getting this to work, we added another
`__forceTContextToBeContravariant`. We also noticed that it made more
sense for BaseContext to be `{}` ("an object with no particular fields")
rather than `Record<string, any>` ("an object where you can do anything
with any of its fields").

We investigated whether the new contravariance annotation coming in the
next two months in TS 4.7
(microsoft/TypeScript#48240) would allow us to
get rid of the `__forceTContextToBeContravariant` hack and the answer is
yes! However, trying `4.7.0-beta` failed for two other reasons. One is
that microsoft/TypeScript#48366 required us to
add some missing `extends` clauses, which we are doing now in this PR.
The other is that `graphql-tools` needs some fixes to work with TS4.7
which we hope they can do soon
(ardatan/graphql-tools#4381).
  • Loading branch information
glasser committed Apr 11, 2022
1 parent 1dec8cc commit 970f3c1
Show file tree
Hide file tree
Showing 11 changed files with 153 additions and 131 deletions.
8 changes: 8 additions & 0 deletions packages/server-types/src/plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ export interface ApolloServerPlugin<TContext extends BaseContext> {
requestDidStart?(
requestContext: GraphQLRequestContext<TContext>,
): Promise<GraphQLRequestListener<TContext> | void>;

// See the similarly named field on ApolloServer for details. Note that it
// appears that this only works if it is a *field*, not a *method*, which is
// why `requestDidStart` (which takes a TContext wrapped in something) is not
// sufficient.
//
// TODO(AS4): Upgrade to TS 4.7 and use `in` instead.
__forceTContextToBeContravariant?: (contextValue: TContext) => void;
}

export interface GraphQLServerListener {
Expand Down
2 changes: 1 addition & 1 deletion packages/server-types/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export type HTTPGraphQLResponse = {
}
);

export type BaseContext = Record<string, any>;
export type BaseContext = {};

export type WithRequired<T, K extends keyof T> = T & Required<Pick<T, K>>;

Expand Down
12 changes: 7 additions & 5 deletions packages/server/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class UnreachableCaseError extends Error {

// TODO(AS4): Move this to its own file or something. Also organize the fields.

export interface ApolloServerInternals<TContext> {
export interface ApolloServerInternals<TContext extends BaseContext> {
formatError?: (error: GraphQLError) => GraphQLFormattedError;
// TODO(AS4): Is there a way (with generics/codegen?) to make
// this "any" more specific? In AS3 there was technically a
Expand Down Expand Up @@ -206,6 +206,8 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
// once `out TContext` is available. Note that when we replace this with `out
// TContext`, we may make it so that users of older TypeScript versions no
// longer have this protection.
//
// TODO(AS4): upgrade to TS 4.7 when it is released and use that instead.
protected __forceTContextToBeContravariant?: (contextValue: TContext) => void;

constructor(config: ApolloServerOptions<TContext>) {
Expand Down Expand Up @@ -636,7 +638,7 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
);
}

private static constructSchema<TContext>(
private static constructSchema<TContext extends BaseContext>(
config: ApolloServerOptionsWithStaticSchema<TContext>,
): GraphQLSchema {
if (config.schema) {
Expand All @@ -659,7 +661,7 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
});
}

private static maybeAddMocksToConstructedSchema<TContext>(
private static maybeAddMocksToConstructedSchema<TContext extends BaseContext>(
schema: GraphQLSchema,
config: ApolloServerOptionsWithStaticSchema<TContext>,
): GraphQLSchema {
Expand Down Expand Up @@ -807,7 +809,7 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
// This is called in the constructor before this.internals has been
// initialized, so we make it static to make it clear it can't assume that
// `this` has been fully initialized.
private static ensurePluginInstantiation<TContext>(
private static ensurePluginInstantiation<TContext extends BaseContext>(
userPlugins: PluginDefinition<TContext>[] = [],
isDev: boolean,
apolloConfig: ApolloConfig,
Expand Down Expand Up @@ -1146,7 +1148,7 @@ export type ImplicitlyInstallablePlugin<TContext extends BaseContext> =
__internal_installed_implicitly__: boolean;
};

export function isImplicitlyInstallablePlugin<TContext>(
export function isImplicitlyInstallablePlugin<TContext extends BaseContext>(
p: ApolloServerPlugin<TContext>,
): p is ImplicitlyInstallablePlugin<TContext> {
return '__internal_installed_implicitly__' in p;
Expand Down
188 changes: 112 additions & 76 deletions packages/server/src/__tests__/ApolloServer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,91 +305,127 @@ describe('ApolloServer executeOperation', () => {
expect(result.data?.contextFoo).toBe('bla');
});

it('typing for context objects works', async () => {
const server = new ApolloServer<{ foo: number }>({
typeDefs: 'type Query { n: Int!, n2: String! }',
resolvers: {
Query: {
n(_parent: any, _args: any, context): number {
return context.foo;
},
n2(_parent: any, _args: any, context): string {
// It knows that context.foo is a number so it doesn't work as a string.
// @ts-expect-error
return context.foo;
describe('context generic typing', () => {
it('typing for context objects works', async () => {
const server = new ApolloServer<{ foo: number }>({
typeDefs: 'type Query { n: Int!, n2: String! }',
resolvers: {
Query: {
n(_parent: any, _args: any, context): number {
return context.foo;
},
n2(_parent: any, _args: any, context): string {
// It knows that context.foo is a number so it doesn't work as a string.
// @ts-expect-error
return context.foo;
},
},
},
},
plugins: [
{
// Works with plugins too!
async requestDidStart({ contextValue }) {
let n: number = contextValue.foo;
// @ts-expect-error
let s: string = contextValue.foo;
// Make sure both variables are used (so the only expected error
// is the type error).
JSON.stringify({ n, s });
plugins: [
{
// Works with plugins too!
async requestDidStart({ contextValue }) {
let n: number = contextValue.foo;
// @ts-expect-error
let s: string = contextValue.foo;
// Make sure both variables are used (so the only expected error
// is the type error).
JSON.stringify({ n, s });
},
},
},
// Plugins declared to be <BaseContext> still work.
ApolloServerPluginCacheControlDisabled(),
],
// Plugins declared to be <BaseContext> still work.
ApolloServerPluginCacheControlDisabled(),
],
});
await server.start();
const result = await server.executeOperation(
{ query: '{ n }' },
{ foo: 123 },
);
expect(result.errors).toBeUndefined();
expect(result.data?.n).toBe(123);

const result2 = await server.executeOperation(
{ query: '{ n }' },
// It knows that context.foo is a number so it doesn't work as a string.
// @ts-expect-error
{ foo: 'asdf' },
);
// GraphQL will be sad that a string was returned from an Int! field.
expect(result2.errors).toBeDefined();
});
await server.start();
const result = await server.executeOperation(
{ query: '{ n }' },
{ foo: 123 },
);
expect(result.errors).toBeUndefined();
expect(result.data?.n).toBe(123);

const result2 = await server.executeOperation(
{ query: '{ n }' },
// It knows that context.foo is a number so it doesn't work as a string.
// This works due to the __forceTContextToBeContravariant hack.
it('context is contravariant', () => {
// @ts-expect-error
{ foo: 'asdf' },
);
// GraphQL will be sad that a string was returned from an Int! field.
expect(result2.errors).toBeDefined();
});
const server1: ApolloServer<{}> = new ApolloServer<{
foo: number;
}>({ typeDefs: 'type Query{id: ID}' });
// avoid the expected error just being an unused variable
expect(server1).toBeDefined();

// The opposite is OK: we can pass a more specific context object to
// something expecting less.
const server2: ApolloServer<{
foo: number;
}> = new ApolloServer<{}>({ typeDefs: 'type Query{id: ID}' });
expect(server2).toBeDefined();
});

// This works due to the __forceTContextToBeContravariant hack.
it('context is contravariant', () => {
// @ts-expect-error
const server1: ApolloServer<{}> = new ApolloServer<{
foo: number;
}>({ typeDefs: 'type Query{id: ID}' });
// avoid the expected error just being an unused variable
expect(server1).toBeDefined();

// The opposite is OK: we can pass a more specific context object to
// something expecting less.
const server2: ApolloServer<{
foo: number;
}> = new ApolloServer<{}>({ typeDefs: 'type Query{id: ID}' });
expect(server2).toBeDefined();
});
it('typing for context objects works with argument to usage reporting', () => {
new ApolloServer<{ foo: number }>({
typeDefs: 'type Query { n: Int! }',
plugins: [
ApolloServerPluginUsageReporting({
generateClientInfo({ contextValue }) {
let n: number = contextValue.foo;
// @ts-expect-error
let s: string = contextValue.foo;
// Make sure both variables are used (so the only expected error
// is the type error).
return {
clientName: `client ${n} ${s}`,
};
},
}),
],
});

it('typing for context objects works with argument to usage reporting', async () => {
new ApolloServer<{ foo: number }>({
typeDefs: 'type Query { n: Int! }',
plugins: [
ApolloServerPluginUsageReporting({
generateClientInfo({ contextValue }) {
let n: number = contextValue.foo;
// @ts-expect-error
let s: string = contextValue.foo;
// Make sure both variables are used (so the only expected error
// is the type error).
return {
clientName: `client ${n} ${s}`,
};
},
}),
],
// Don't start the server because we don't actually want any usage reporting.
});

// Don't start the server because we don't actually want any usage reporting.
it('typing for plugins works appropriately', () => {
type SpecificContext = { someField: boolean };

function takesPlugin<TContext extends BaseContext>(
_p: ApolloServerPlugin<TContext>,
) {}

const specificPlugin: ApolloServerPlugin<SpecificContext> = {
async requestDidStart({ contextValue }) {
console.log(contextValue.someField); // this doesn't actually run
},
};

const basePlugin: ApolloServerPlugin<BaseContext> = {
async requestDidStart({ contextValue }) {
console.log(contextValue); // this doesn't actually run
},
};

// @ts-expect-error
takesPlugin<BaseContext>(specificPlugin);
takesPlugin<SpecificContext>(basePlugin);

new ApolloServer<BaseContext>({
typeDefs: 'type Query { x: ID }',
// @ts-expect-error
plugins: [specificPlugin],
});
new ApolloServer<SpecificContext>({
typeDefs: 'type Query { x: ID }',
plugins: [basePlugin],
});
});
});
});
7 changes: 5 additions & 2 deletions packages/server/src/httpBatching.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import type {
BaseContext,
HTTPGraphQLRequest,
HTTPGraphQLResponse,
} from '@apollo/server-types';
import type { ApolloServerInternals, SchemaDerivedData } from './ApolloServer';
import { HeaderMap, HttpQueryError, runHttpQuery } from './runHttpQuery';

export async function runBatchHttpQuery<TContext>(
export async function runBatchHttpQuery<TContext extends BaseContext>(
batchRequest: Omit<HTTPGraphQLRequest, 'body'> & { body: any[] },
contextValue: TContext,
schemaDerivedData: SchemaDerivedData,
Expand Down Expand Up @@ -53,7 +54,9 @@ export async function runBatchHttpQuery<TContext>(
return combinedResponse;
}

export async function runPotentiallyBatchedHttpQuery<TContext>(
export async function runPotentiallyBatchedHttpQuery<
TContext extends BaseContext,
>(
httpGraphQLRequest: HTTPGraphQLRequest,
contextValue: TContext,
schemaDerivedData: SchemaDerivedData,
Expand Down
10 changes: 0 additions & 10 deletions packages/server/src/plugin/cacheControl/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,3 @@ function cacheAnnotationFromField(
function isRestricted(hint: CacheHint) {
return hint.maxAge !== undefined || hint.scope !== undefined;
}

// This plugin does nothing, but it ensures that ApolloServer won't try
// to add a default ApolloServerPluginCacheControl.
export function ApolloServerPluginCacheControlDisabled(): InternalApolloServerPlugin<BaseContext> {
return {
__internal_plugin_id__() {
return 'CacheControl';
},
};
}
27 changes: 17 additions & 10 deletions packages/server/src/plugin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@
// The goal is that the generated `dist/plugin/index.js` file has no top-level
// require calls.
import type { ApolloServerPlugin, BaseContext } from '@apollo/server-types';
import type {
InternalApolloServerPlugin,
InternalPluginId,
} from '../internalPlugin';

function disabledPlugin(id: InternalPluginId): ApolloServerPlugin<BaseContext> {
const plugin: InternalApolloServerPlugin<BaseContext> = {
__internal_plugin_id__() {
return id;
},
};
return plugin;
}

//#region Usage reporting
import type { ApolloServerPluginUsageReportingOptions } from './usageReporting';
Expand All @@ -35,7 +48,7 @@ export function ApolloServerPluginUsageReporting<TContext extends BaseContext>(
return require('./usageReporting').ApolloServerPluginUsageReporting(options);
}
export function ApolloServerPluginUsageReportingDisabled(): ApolloServerPlugin<BaseContext> {
return require('./usageReporting').ApolloServerPluginUsageReportingDisabled();
return disabledPlugin('UsageReporting');
}
//#endregion

Expand All @@ -62,7 +75,7 @@ export function ApolloServerPluginInlineTrace(
return require('./inlineTrace').ApolloServerPluginInlineTrace(options);
}
export function ApolloServerPluginInlineTraceDisabled(): ApolloServerPlugin<BaseContext> {
return require('./inlineTrace').ApolloServerPluginInlineTraceDisabled();
return disabledPlugin('InlineTrace');
}
//#endregion

Expand All @@ -76,7 +89,7 @@ export function ApolloServerPluginCacheControl(
return require('./cacheControl').ApolloServerPluginCacheControl(options);
}
export function ApolloServerPluginCacheControlDisabled(): ApolloServerPlugin<BaseContext> {
return require('./cacheControl').ApolloServerPluginCacheControlDisabled();
return disabledPlugin('CacheControl');
}
//#endregion

Expand All @@ -93,14 +106,8 @@ export function ApolloServerPluginDrainHttpServer(
//#endregion

//#region LandingPage
import type { InternalApolloServerPlugin } from '../internalPlugin';
export function ApolloServerPluginLandingPageDisabled(): ApolloServerPlugin<BaseContext> {
const plugin: InternalApolloServerPlugin<BaseContext> = {
__internal_plugin_id__() {
return 'LandingPageDisabled';
},
};
return plugin;
return disabledPlugin('LandingPageDisabled');
}

import type {
Expand Down
10 changes: 0 additions & 10 deletions packages/server/src/plugin/inlineTrace/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,13 +129,3 @@ export function ApolloServerPluginInlineTrace(
},
};
}

// This plugin does nothing, but it ensures that ApolloServer won't try
// to add a default ApolloServerPluginInlineTrace.
export function ApolloServerPluginInlineTraceDisabled(): InternalApolloServerPlugin<BaseContext> {
return {
__internal_plugin_id__() {
return 'InlineTrace';
},
};
}
Loading

0 comments on commit 970f3c1

Please sign in to comment.