Remove access to process.env from hot code paths (#5657)

Co-authored-by: David Glasser <glasser@davidglasser.net>
apollographql · Sep 30, 2021 · c2a3347 · c2a3347
1 parent ed14cbc
commit c2a3347
Show file tree

Hide file tree

Showing 15 changed files with 67 additions and 65 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 ## vNEXT
 
+- `apollo-server-core`: For ease of testing, you can specify the node environment via `new ApolloServer({nodeEnv})` in addition to via the `NODE_ENV` environment variable. The environment variable is now only read during server startup (and in some error cases) rather than on every request. [PR #5657](https://github.com/apollographql/apollo-server/pull/5657)
 - `apollo-server-koa`: The peer dependency on `koa` (added in v3.0.0) should be a `^` range dependency rather than depending on exactly one version, and it should not be automatically increased when new versions of `koa` are released. [PR #5759](https://github.com/apollographql/apollo-server/pull/5759)
 - `apollo-server-fastify`: Export `ApolloServerFastifyConfig` and `FastifyContext` TypeScript types. [PR #5743](https://github.com/apollographql/apollo-server/pull/5743)
 - `apollo-server-core`: Only generate the schema hash once on startup rather than twice. [PR #5757](https://github.com/apollographql/apollo-server/pull/5757)

diff --git a/docs/source/api/apollo-server.md b/docs/source/api/apollo-server.md
@@ -390,6 +390,19 @@ The default value is `true`. Set this to `false` to use mocked resolvers only fo
 </td>
 </tr>
 
+<tr>
+<td>
+
+##### `nodeEnv`
+
+`String`
+</td>
+<td>
+
+If this is set to any string value, use that value instead of the environment variable `NODE_ENV` for the features whose defaults depend on `NODE_ENV` (like the [`debug`](#introspection) and [`introspection`](#introspection) options). Note that passing the empty string here is equivalent to running with the `NODE_ENV` environment variable unset. This is primarily meant for testing the effects of the `NODE_ENV` environment variable.
+</td>
+</tr>
+
 </tbody>
 </table>
 

diff --git a/packages/apollo-datasource-rest/src/RESTDataSource.ts b/packages/apollo-datasource-rest/src/RESTDataSource.ts
@@ -44,6 +44,8 @@ export interface CacheOptions {
 export type Body = BodyInit | object;
 export { Request };
 
+const NODE_ENV = process.env.NODE_ENV;
+
 export abstract class RESTDataSource<TContext = any> extends DataSource {
   httpCache!: HTTPCache;
   context!: TContext;
@@ -282,7 +284,7 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
     request: Request,
     fn: () => Promise<TResult>,
   ): Promise<TResult> {
-    if (process.env.NODE_ENV === 'development') {
+    if (NODE_ENV === 'development') {
       // We're not using console.time because that isn't supported on Cloudflare
       const startTime = Date.now();
       try {

diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
@@ -151,7 +151,10 @@ export class ApolloServerBase<
   // The constructor should be universal across all environments. All environment specific behavior should be set by adding or overriding methods
   constructor(config: Config<ContextFunctionParams>) {
     if (!config) throw new Error('ApolloServer requires options.');
-    this.config = config;
+    this.config = {
+      ...config,
+      nodeEnv: config.nodeEnv ?? process.env.NODE_ENV,
+    };
     const {
       context,
       resolvers,
@@ -170,7 +173,7 @@ export class ApolloServerBase<
       mockEntireSchema,
       experimental_approximateDocumentStoreMiB,
       ...requestOptions
-    } = config;
+    } = this.config;
 
     // Setup logging facilities
     if (config.logger) {
@@ -206,16 +209,7 @@ export class ApolloServerBase<
     this.experimental_approximateDocumentStoreMiB =
       experimental_approximateDocumentStoreMiB;
 
-    // Allow tests to override process.env.NODE_ENV. As a bonus, this means
-    // we're only reading the env var once in the constructor, which is faster
-    // than reading it over and over as each read is a syscall. Note that an
-    // explicit `__testing_nodeEnv__: undefined` overrides a set environment
-    // variable!
-    const nodeEnv =
-      '__testing_nodeEnv__' in config
-        ? config.__testing_nodeEnv__
-        : process.env.NODE_ENV;
-    const isDev = nodeEnv !== 'production';
+    const isDev = this.config.nodeEnv !== 'production';
 
     // We handle signals if it was explicitly requested, or if we're in Node,
     // not in a test, not in a serverless framework, and it wasn't explicitly
@@ -224,7 +218,9 @@ export class ApolloServerBase<
     this.stopOnTerminationSignals =
       typeof stopOnTerminationSignals === 'boolean'
         ? stopOnTerminationSignals
-        : isNodeLike && nodeEnv !== 'test' && !this.serverlessFramework();
+        : isNodeLike &&
+          this.config.nodeEnv !== 'test' &&
+          !this.serverlessFramework();
 
     // if this is local dev, introspection should turned on
     // in production, we can manually turn introspection on by passing {

diff --git a/packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts b/packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts
@@ -29,6 +29,7 @@ describe('runHttpQuery', () => {
         query: '{ testString }',
       },
       options: {
+        debug: false,
         schema,
         schemaHash: generateSchemaHash(schema),
       },

diff --git a/packages/apollo-server-core/src/graphqlOptions.ts b/packages/apollo-server-core/src/graphqlOptions.ts
@@ -58,7 +58,7 @@ export interface GraphQLServerOptions<
   plugins?: ApolloServerPlugin[];
   documentStore?: InMemoryLRUCache<DocumentNode>;
   parseOptions?: ParseOptions;
-  __testing_nodeEnv__?: string | undefined;
+  nodeEnv?: string;
 }
 
 export type DataSources<TContext> = {

diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts
@@ -114,11 +114,13 @@ export function throwHttpGraphQLError<E extends Error>(
   );
 }
 
+const NODE_ENV = process.env.NODE_ENV ?? '';
+
 export async function runHttpQuery(
   handlerArguments: Array<any>,
   request: HttpQueryRequest,
 ): Promise<HttpQueryResponse> {
-  function debugFromNodeEnv(nodeEnv: string | undefined) {
+  function debugFromNodeEnv(nodeEnv: string = NODE_ENV) {
     return nodeEnv !== 'production' && nodeEnv !== 'test';
   }
 
@@ -129,18 +131,15 @@ export async function runHttpQuery(
     // The options can be generated asynchronously, so we don't have access to
     // the normal options provided by the user, such as: formatError,
     // debug. Therefore, we need to do some unnatural things, such
-    // as use NODE_ENV to determine the debug settings
+    // as use NODE_ENV to determine the debug settings. Please note that this
+    // will not be sensitive to any runtime changes made to NODE_ENV.
     return throwHttpGraphQLError(500, [e as Error], {
-      debug: debugFromNodeEnv(process.env.NODE_ENV),
+      debug: debugFromNodeEnv(),
     });
   }
 
   if (options.debug === undefined) {
-    const nodeEnv =
-      '__testing_nodeEnv__' in options
-        ? options.__testing_nodeEnv__
-        : process.env.NODE_ENV;
-    options.debug = debugFromNodeEnv(nodeEnv);
+    options.debug = debugFromNodeEnv(options.nodeEnv);
   }
 
   // TODO: Errors thrown while resolving the context in

diff --git a/packages/apollo-server-core/src/types.ts b/packages/apollo-server-core/src/types.ts
@@ -99,12 +99,5 @@ export interface Config<ContextFunctionParams = any> extends BaseConfig {
   experimental_approximateDocumentStoreMiB?: number;
   stopOnTerminationSignals?: boolean;
   apollo?: ApolloConfigInput;
-  // Apollo Server only uses process.env.NODE_ENV to determine defaults for
-  // other behavior which have other mechanisms of setting explicitly. Sometimes
-  // our tests want to test the exact logic of how NODE_ENV affects defaults;
-  // they can set this parameter, but there's no reason to do so other than for
-  // tests. Note that an explicit `__testing_nodeEnv__: undefined` means "act as
-  // if the environment variable is not set", whereas the absence of
-  // `__testing_nodeEnv__` means to honor the environment variable.
-  __testing_nodeEnv__?: string | undefined;
+  nodeEnv?: string;
 }
diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts
@@ -120,7 +120,7 @@ describe('apollo-server-express', () => {
       const { httpServer } = await createServer({
         typeDefs,
         resolvers,
-        __testing_nodeEnv__: undefined, // default landing page
+        nodeEnv: '', // default landing page
       });
 
       await request(httpServer)
@@ -265,7 +265,7 @@ describe('apollo-server-express', () => {
             throw new AuthenticationError('valid result');
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -296,7 +296,7 @@ describe('apollo-server-express', () => {
             },
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -326,7 +326,7 @@ describe('apollo-server-express', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -355,7 +355,7 @@ describe('apollo-server-express', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });

diff --git a/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-fastify/src/__tests__/ApolloServer.test.ts
@@ -178,7 +178,7 @@ describe('apollo-server-fastify', () => {
       const { httpServer } = await createServer({
         typeDefs,
         resolvers,
-        __testing_nodeEnv__: undefined, // default landing page
+        nodeEnv: '', // default landing page
       });
 
       await request(httpServer)
@@ -283,7 +283,7 @@ describe('apollo-server-fastify', () => {
             throw new AuthenticationError('valid result');
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -314,7 +314,7 @@ describe('apollo-server-fastify', () => {
             },
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -344,7 +344,7 @@ describe('apollo-server-fastify', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -373,7 +373,7 @@ describe('apollo-server-fastify', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });

diff --git a/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-hapi/src/__tests__/ApolloServer.test.ts
@@ -116,7 +116,7 @@ describe('non-integration tests', () => {
       const { httpServer } = await createServer({
         typeDefs,
         resolvers,
-        __testing_nodeEnv__: undefined, // default landing page
+        nodeEnv: '', // default landing page
       });
 
       await request(httpServer)
@@ -288,7 +288,7 @@ describe('non-integration tests', () => {
             throw new AuthenticationError('valid result');
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -319,7 +319,7 @@ describe('non-integration tests', () => {
             },
           },
           // Stack trace not included for NODE_ENV=test
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -349,7 +349,7 @@ describe('non-integration tests', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -378,7 +378,7 @@ describe('non-integration tests', () => {
               },
             },
           },
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });

diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts
@@ -248,7 +248,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
           const { url: uri } = await createApolloServer({
             schema,
             stopOnTerminationSignals: false,
-            __testing_nodeEnv__: undefined,
+            nodeEnv: '',
           });
 
           const apolloFetch = createApolloFetch({ uri });
@@ -262,7 +262,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
           const { url: uri } = await createApolloServer({
             schema,
             stopOnTerminationSignals: false,
-            __testing_nodeEnv__: 'production',
+            nodeEnv: 'production',
           });
 
           const apolloFetch = createApolloFetch({ uri });
@@ -281,7 +281,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
             schema,
             introspection: true,
             stopOnTerminationSignals: false,
-            __testing_nodeEnv__: 'production',
+            nodeEnv: 'production',
           });
 
           const apolloFetch = createApolloFetch({ uri });
@@ -979,7 +979,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
               ],
               debug: true,
               stopOnTerminationSignals: false,
-              __testing_nodeEnv__: undefined,
+              nodeEnv: '',
               ...constructorOptions,
             });
 
@@ -1595,7 +1595,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
               typeDefs,
               resolvers,
               stopOnTerminationSignals: false,
-              __testing_nodeEnv__: undefined,
+              nodeEnv: '',
               context: () => {
                 throw new AuthenticationError('valid result');
               },
@@ -1653,7 +1653,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
             },
           },
           stopOnTerminationSignals: false,
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -1683,7 +1683,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
             },
           },
           stopOnTerminationSignals: false,
-          __testing_nodeEnv__: 'production',
+          nodeEnv: 'production',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -1714,7 +1714,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
             },
           },
           stopOnTerminationSignals: false,
-          __testing_nodeEnv__: 'development',
+          nodeEnv: 'development',
         });
 
         const apolloFetch = createApolloFetch({ uri });
@@ -2922,7 +2922,7 @@ export function testApolloServer<AS extends ApolloServerBase>(
             })),
           ],
           // dev mode, so we get the local landing page
-          __testing_nodeEnv__: undefined,
+          nodeEnv: '',
         };
       }