fix(nextjs): Stop excluding withSentryConfig from serverless bundles (#6267)

Co-authored-by: Luca Forstner <[email protected]>

Author: lobsterkatie

packages/nextjs/rollup.npm.config.js

@@ -5,7 +5,12 @@ export default [
     makeBaseNPMConfig({
       // We need to include `instrumentServer.ts` separately because it's only conditionally required, and so rollup
       // doesn't automatically include it when calculating the module dependency tree.
-      entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/utils/instrumentServer.ts'],
+      entrypoints: [
+        'src/index.server.ts',
+        'src/index.client.ts',
+        'src/utils/instrumentServer.ts',
+        'src/config/webpack.ts',
+      ],
 
       // prevent this internal nextjs code from ending up in our built package (this doesn't happen automatially because
       // the name doesn't match an SDK dependency)

packages/nextjs/src/config/webpack.ts

@@ -1,6 +1,13 @@
 /* eslint-disable max-lines */
 import { getSentryRelease } from '@sentry/node';
-import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils';
+import {
+  arrayify,
+  dropUndefinedKeys,
+  escapeStringForRegex,
+  GLOBAL_OBJ,
+  logger,
+  stringMatchesSomePattern,
+} from '@sentry/utils';
 import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin';
 import * as chalk from 'chalk';
 import * as fs from 'fs';
@@ -27,6 +34,14 @@ export { SentryWebpackPlugin };
 // TODO: merge default SentryWebpackPlugin include with their SentryWebpackPlugin include
 // TODO: drop merged keys from override check? `includeDefaults` option?
 
+// In order to make sure that build-time code isn't getting bundled in runtime bundles (specifically, in the serverless
+// functions into which nextjs converts a user's routes), we exclude this module (and all of its dependencies) from the
+// nft file manifests nextjs generates. (See the code dealing with the `TraceEntryPointsPlugin` below.) So that we don't
+// crash people, we therefore need to make sure nothing tries to either require this file or import from it. Setting
+// this env variable allows us to test whether or not that's happened.
+const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & { _sentryWebpackModuleLoaded?: true };
+_global._sentryWebpackModuleLoaded = true;
+
 /**
  * Construct the function which will be used as the nextjs config's `webpack` value.
  *
@@ -106,14 +121,15 @@ export function constructWebpackConfigFunction(
 
       // Prevent `@vercel/nft` (which nextjs uses to determine which files are needed when packaging up a lambda) from
       // including any of our build-time code or dependencies. (Otherwise it'll include files like this one and even the
-      // entirety of rollup and sucrase.)
+      // entirety of rollup and sucrase.) Since this file is the root of that dependency tree, it's enough to just
+      // exclude it and the rest will be excluded as well.
       const nftPlugin = newConfig.plugins?.find((plugin: WebpackPluginInstance) => {
         const proto = Object.getPrototypeOf(plugin) as WebpackPluginInstance;
         return proto.constructor.name === 'TraceEntryPointsPlugin';
       }) as WebpackPluginInstance & { excludeFiles: string[] };
       if (nftPlugin) {
         if (Array.isArray(nftPlugin.excludeFiles)) {
-          nftPlugin.excludeFiles.push(path.join(__dirname, 'withSentryConfig.js'));
+          nftPlugin.excludeFiles.push(__filename);
         } else {
           __DEBUG_BUILD__ &&
             logger.warn(

packages/nextjs/src/config/withSentryConfig.ts

@@ -1,5 +1,11 @@
-import type { ExportedNextConfig, NextConfigFunction, NextConfigObject, SentryWebpackPluginOptions } from './types';
-import { constructWebpackConfigFunction } from './webpack';
+import { isBuild } from '../utils/isBuild';
+import type {
+  ExportedNextConfig,
+  NextConfigFunction,
+  NextConfigObject,
+  NextConfigObjectWithSentry,
+  SentryWebpackPluginOptions,
+} from './types';
 
 /**
  * Add Sentry options to the config to be exported from the user's `next.config.js` file.
@@ -16,38 +22,42 @@ export function withSentryConfig(
   // `defaults` in order to pass them along to the user's function
   if (typeof exportedUserNextConfig === 'function') {
     return function (phase: string, defaults: { defaultConfig: NextConfigObject }): NextConfigObject {
-      let userNextConfigObject = exportedUserNextConfig(phase, defaults);
+      const userNextConfigObject = exportedUserNextConfig(phase, defaults);
 
-      // Next 12.2.3+ warns about non-canonical properties on `userNextConfig`, so grab and then remove the `sentry`
-      // property there. Where we actually need it is in the webpack config function we're going to create, so pass it
-      // to `constructWebpackConfigFunction` so that it will be in the created function's closure.
-      const { sentry: userSentryOptions } = userNextConfigObject;
-      delete userNextConfigObject.sentry;
-      // Remind TS that there's now no `sentry` property
-      userNextConfigObject = userNextConfigObject as NextConfigObject;
-
-      return {
-        ...userNextConfigObject,
-        webpack: constructWebpackConfigFunction(
-          userNextConfigObject,
-          userSentryWebpackPluginOptions,
-          userSentryOptions,
-        ),
-      };
+      return getFinalConfigObject(userNextConfigObject, userSentryWebpackPluginOptions);
     };
   }
 
   // Otherwise, we can just merge their config with ours and return an object.
+  return getFinalConfigObject(exportedUserNextConfig, userSentryWebpackPluginOptions);
+}
 
-  // Prevent nextjs from getting mad about having a non-standard config property in `userNextConfig`. (See note above
-  // for a more thorough explanation of what we're doing here.)
-  const { sentry: userSentryOptions } = exportedUserNextConfig;
-  delete exportedUserNextConfig.sentry;
+// Modify the materialized object form of the user's next config by deleting the `sentry` property and wrapping the
+// `webpack` property
+function getFinalConfigObject(
+  incomingUserNextConfigObject: NextConfigObjectWithSentry,
+  userSentryWebpackPluginOptions: Partial<SentryWebpackPluginOptions>,
+): NextConfigObject {
+  // Next 12.2.3+ warns about non-canonical properties on `userNextConfig`, so grab and then remove the `sentry`
+  // property there. Where we actually need it is in the webpack config function we're going to create, so pass it
+  // to `constructWebpackConfigFunction` so that it can live in the returned function's closure.
+  const { sentry: userSentryOptions } = incomingUserNextConfigObject;
+  delete incomingUserNextConfigObject.sentry;
   // Remind TS that there's now no `sentry` property
-  const userNextConfigObject = exportedUserNextConfig as NextConfigObject;
+  const userNextConfigObject = incomingUserNextConfigObject as NextConfigObject;
+
+  // In order to prevent all of our build-time code from being bundled in people's route-handling serverless functions,
+  // we exclude `webpack.ts` and all of its dependencies from nextjs's `@vercel/nft` filetracing. We therefore need to
+  // make sure that we only require it at build time.
+  if (isBuild()) {
+    // eslint-disable-next-line @typescript-eslint/no-var-requires
+    const { constructWebpackConfigFunction } = require('./webpack');
+    return {
+      ...userNextConfigObject,
+      webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions),
+    };
+  }
 
-  return {
-    ...userNextConfigObject,
-    webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions),
-  };
+  // At runtime, we just return the user's config untouched.
+  return userNextConfigObject;
 }

packages/nextjs/test/buildProcess/tests/nft.test.ts

@@ -11,12 +11,15 @@ it('excludes build-time SDK dependencies from nft files', () => {
 
   // These are all of the files which control the way we modify the user's app build by changing the webpack config.
   // They get mistakenly included by the nft plugin because we export `withSentryConfig` from `index.server.ts`. Because
-  // the wrappers are referenced from the code we inject at build time, they need to stay.
+  // the wrappers are referenced from the code we inject at build time, they need to stay. `withSentryConfig.ts` itself
+  // also needs to stay because nextjs loads `next.config.js` at runtime
   const sentryConfigDirEntries = dogNFTjson.files.filter(entry =>
     entry.includes('node_modules/@sentry/nextjs/build/cjs/config'),
   );
-  const withSentryConfigEntries = sentryConfigDirEntries.filter(entry => !entry.includes('config/wrappers'));
   const sentryWrapperEntries = sentryConfigDirEntries.filter(entry => entry.includes('config/wrappers'));
+  const excludedSentryConfigEntries = sentryConfigDirEntries.filter(
+    entry => !sentryWrapperEntries.includes(entry) && !entry.includes('withSentryConfig'),
+  );
 
   // Sucrase and rollup are dependencies of one of the webpack loaders we add to the config - also not needed at runtime.
   const sucraseEntries = dogNFTjson.files.filter(entry => entry.includes('node_modules/sucrase'));
@@ -25,7 +28,7 @@ it('excludes build-time SDK dependencies from nft files', () => {
   );
 
   // None of the build-time dependencies should be listed
-  expect(withSentryConfigEntries.length).toEqual(0);
+  expect(excludedSentryConfigEntries.length).toEqual(0);
   expect(sucraseEntries.length).toEqual(0);
   expect(rollupEntries.length).toEqual(0);
 

packages/nextjs/test/config/index.test.ts renamed to packages/nextjs/test/config/withSentryConfig.test.ts

@@ -1,6 +1,9 @@
+import * as isBuildModule from '../../src/utils/isBuild';
 import { defaultsObject, exportedNextConfig, runtimePhase, userNextConfig } from './fixtures';
 import { materializeFinalNextConfig } from './testUtils';
 
+const isBuildSpy = jest.spyOn(isBuildModule, 'isBuild').mockReturnValue(true);
+
 describe('withSentryConfig', () => {
   it('includes expected properties', () => {
     const finalConfig = materializeFinalNextConfig(exportedNextConfig);
@@ -59,4 +62,37 @@ describe('withSentryConfig', () => {
     // directly
     expect('sentry' in finalConfig).toBe(false);
   });
+
+  describe('conditional use of `constructWebpackConfigFunction`', () => {
+    // Note: In these tests, it would be nice to be able to spy on `constructWebpackConfigFunction` to see whether or
+    // not it's called, but that sets up a catch-22: If you import or require the module to spy on the function, it gets
+    // cached and the `require` call we care about (inside of `withSentryConfig`) doesn't actually run the module code.
+    // Alternatively, if we call `jest.resetModules()` after setting up the spy, then the module code *is* run a second
+    // time, but the spy belongs to the first instance of the module and therefore never registers a call. Thus we have
+    // to test whether or not the file is required instead.
+
+    it('imports from `webpack.ts` if `isBuild` returns true', () => {
+      jest.isolateModules(() => {
+        // In case this is still set from elsewhere, reset it
+        delete (global as any)._sentryWebpackModuleLoaded;
+
+        materializeFinalNextConfig(exportedNextConfig);
+
+        expect((global as any)._sentryWebpackModuleLoaded).toBe(true);
+      });
+    });
+
+    it("doesn't import from `webpack.ts` if `isBuild` returns false", () => {
+      jest.isolateModules(() => {
+        isBuildSpy.mockReturnValueOnce(false);
+
+        // In case this is still set from elsewhere, reset it
+        delete (global as any)._sentryWebpackModuleLoaded;
+
+        materializeFinalNextConfig(exportedNextConfig);
+
+        expect((global as any)._sentryWebpackModuleLoaded).toBeUndefined();
+      });
+    });
+  });
 });