Move Disabled plugins up a level and change some context typing

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).

Author: glasser

packages/server-types/src/plugins.ts

@@ -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 {

packages/server-types/src/types.ts

@@ -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>>;
 

packages/server/src/ApolloServer.ts

@@ -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
@@ -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>) {
@@ -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) {
@@ -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 {
@@ -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,
@@ -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;

packages/server/src/__tests__/ApolloServer.test.ts

@@ -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],
+      });
+    });
   });
 });

packages/server/src/httpBatching.ts

@@ -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,
@@ -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,

packages/server/src/plugin/cacheControl/index.ts

@@ -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';
-    },
-  };
-}

packages/server/src/plugin/index.ts

@@ -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';
@@ -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
 
@@ -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
 
@@ -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
 
@@ -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 {

packages/server/src/plugin/inlineTrace/index.ts

@@ -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';
-    },
-  };
-}