Move default plugin setup to start() (#6555)

We've added an addPlugin API that lets you add plugins after the
constructor, so it's no longer appropriate to do "have you added a usage
reporting plugin?" checks in the constructor. Instead we do it in
start().

This also means this code can be async, which means we can have it do
runtime deep imports soon...

While changing a test for this, add a bunch of missing server.stop()s to
tests.

Fixes #6552.

packages/server/src/ApolloServer.ts

@@ -37,7 +37,6 @@ import type {
   HTTPGraphQLRequest,
   HTTPGraphQLResponse,
   LandingPage,
-  PluginDefinition,
   ApolloConfig,
   ApolloServerOptions,
   DocumentStore,
@@ -228,13 +227,18 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
 
     const isDev = nodeEnv !== 'production';
 
-    // Plugins will be instantiated if they aren't already, and this.plugins
-    // is populated accordingly.
-    const plugins = ApolloServer.ensurePluginInstantiation(
-      config.plugins,
-      isDev,
-      apolloConfig,
-      logger,
+    // Plugins can be (for some reason) provided as a function, which we have to
+    // call first to get the actual plugin. Note that more plugins can be added
+    // before `start()` with `addPlugin()` (eg, plugins that want to take this
+    // ApolloServer as an argument), and `start()` will call
+    // `ensurePluginInstantiation` to add default plugins.
+    const plugins: ApolloServerPlugin<TContext>[] = (config.plugins ?? []).map(
+      (plugin) => {
+        if (typeof plugin === 'function') {
+          return plugin();
+        }
+        return plugin;
+      },
     );
 
     const state: ServerState<TContext> = config.gateway
@@ -389,6 +393,10 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
       startedInBackground,
     };
     try {
+      // Now that you can't call addPlugin any more, add default plugins like
+      // usage reporting if they're not already added.
+      this.addDefaultPlugins();
+
       const toDispose: (() => Promise<void>)[] = [];
       const executor = await schemaManager.start();
       if (executor) {
@@ -845,21 +853,9 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
     this.internals.state = { phase: 'stopped', stopError: null };
   }
 
-  // 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 extends BaseContext>(
-    userPlugins: PluginDefinition<TContext>[] = [],
-    isDev: boolean,
-    apolloConfig: ApolloConfig,
-    logger: Logger,
-  ): ApolloServerPlugin<TContext>[] {
-    const plugins = userPlugins.map((plugin) => {
-      if (typeof plugin === 'function') {
-        return plugin();
-      }
-      return plugin;
-    });
+  private addDefaultPlugins() {
+    const { plugins, apolloConfig, logger, nodeEnv } = this.internals;
+    const isDev = nodeEnv !== 'production';
 
     const alreadyHavePluginWithInternalId = (id: InternalPluginId) =>
       plugins.some(
@@ -959,8 +955,6 @@ export class ApolloServer<TContext extends BaseContext = BaseContext> {
       plugin.__internal_installed_implicitly__ = true;
       plugins.push(plugin);
     }
-
-    return plugins;
   }
 
   public addPlugin(plugin: ApolloServerPlugin<TContext>) {

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

@@ -52,24 +52,24 @@ describe('ApolloServer construction', () => {
     ).not.toThrow();
   });
 
-  it('succeeds when passed a graphVariant in construction', () => {
+  it('succeeds when passed a graphVariant in construction', async () => {
     const logger = mockLogger();
-    expect(
-      () =>
-        new ApolloServer({
-          typeDefs,
-          resolvers,
-          apollo: {
-            graphVariant: 'foo',
-            key: 'service:real:key',
-          },
-          logger,
-        }),
-    ).not.toThrow();
+    const server = new ApolloServer({
+      typeDefs,
+      resolvers,
+      apollo: {
+        graphVariant: 'foo',
+        key: 'service:real:key',
+      },
+      logger,
+    });
+    expect(logger.warn).toHaveBeenCalledTimes(0);
+    await server.start();
     expect(logger.warn).toHaveBeenCalledTimes(1);
     expect(logger.warn.mock.calls[0][0]).toMatch(
       /Apollo key but have not specified a graph ref/,
     );
+    await server.stop();
   });
 
   it('throws when a GraphQLSchema is not provided to the schema configuration option', () => {
@@ -255,6 +255,7 @@ describe('ApolloServer executeOperation', () => {
     expect(result.errors?.[0].extensions).toStrictEqual({
       code: 'INTERNAL_SERVER_ERROR',
     });
+    await server.stop();
   });
 
   it('returns error information with details when debug is enabled', async () => {
@@ -273,6 +274,7 @@ describe('ApolloServer executeOperation', () => {
     const extensions = result.errors?.[0].extensions;
     expect(extensions).toHaveProperty('code', 'INTERNAL_SERVER_ERROR');
     expect(extensions).toHaveProperty('exception.stacktrace');
+    await server.stop();
   });
 
   it('works with string', async () => {
@@ -285,6 +287,7 @@ describe('ApolloServer executeOperation', () => {
     const { result } = await server.executeOperation({ query: '{ hello }' });
     expect(result.errors).toBeUndefined();
     expect(result.data?.hello).toBe('world');
+    await server.stop();
   });
 
   it('works with AST', async () => {
@@ -303,6 +306,7 @@ describe('ApolloServer executeOperation', () => {
     });
     expect(result.errors).toBeUndefined();
     expect(result.data?.hello).toBe('world');
+    await server.stop();
   });
 
   it('parse errors', async () => {
@@ -315,6 +319,7 @@ describe('ApolloServer executeOperation', () => {
     const { result } = await server.executeOperation({ query: '{' });
     expect(result.errors).toHaveLength(1);
     expect(result.errors?.[0].extensions?.code).toBe('GRAPHQL_PARSE_FAILED');
+    await server.stop();
   });
 
   it('passes its second argument as context object', async () => {
@@ -330,6 +335,7 @@ describe('ApolloServer executeOperation', () => {
     );
     expect(result.errors).toBeUndefined();
     expect(result.data?.contextFoo).toBe('bla');
+    await server.stop();
   });
 
   describe('context generic typing', () => {
@@ -380,6 +386,7 @@ describe('ApolloServer executeOperation', () => {
       );
       // GraphQL will be sad that a string was returned from an Int! field.
       expect(result2.errors).toBeDefined();
+      await server.stop();
     });
 
     // This works due to the __forceTContextToBeContravariant hack.
@@ -482,5 +489,6 @@ describe('ApolloServer addPlugin', () => {
     ).toThrowErrorMatchingInlineSnapshot(
       `"Can't add plugins after the server has started"`,
     );
+    await server.stop();
   });
 });