ref(nextjs): Remove NFT build time exclusions (#6846)

Author: Luca Forstner

packages/nextjs/src/config/webpack.ts

@@ -1,14 +1,7 @@
 /* eslint-disable complexity */
 /* eslint-disable max-lines */
 import { getSentryRelease } from '@sentry/node';
-import {
-  arrayify,
-  dropUndefinedKeys,
-  escapeStringForRegex,
-  GLOBAL_OBJ,
-  logger,
-  stringMatchesSomePattern,
-} from '@sentry/utils';
+import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger, stringMatchesSomePattern } from '@sentry/utils';
 import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin';
 import * as chalk from 'chalk';
 import * as fs from 'fs';
@@ -27,7 +20,6 @@ import type {
   WebpackConfigObjectWithModuleRules,
   WebpackEntryProperty,
   WebpackModuleRule,
-  WebpackPluginInstance,
 } from './types';
 
 export { SentryWebpackPlugin };
@@ -36,14 +28,6 @@ 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.
  *
@@ -159,31 +143,6 @@ 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.) 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(__filename);
-        } else {
-          __DEBUG_BUILD__ &&
-            logger.warn(
-              'Unable to exclude Sentry build-time helpers from nft files. `TraceEntryPointsPlugin.excludeFiles` is not ' +
-                'an array. This is a bug; please report this to Sentry:  https://github.com/getsentry/sentry-javascript/issues/.',
-            );
-        }
-      } else {
-        __DEBUG_BUILD__ &&
-          logger.warn(
-            'Unable to exclude Sentry build-time helpers from nft files. Could not find `TraceEntryPointsPlugin`.',
-          );
-      }
     }
 
     // The SDK uses syntax (ES6 and ES6+ features like object spread) which isn't supported by older browsers. For users

packages/nextjs/src/config/withSentryConfig.ts

@@ -1,5 +1,3 @@
-import { PHASE_DEVELOPMENT_SERVER, PHASE_PRODUCTION_BUILD } from 'next/constants';
-
 import type {
   ExportedNextConfig,
   NextConfigFunction,
@@ -8,6 +6,7 @@ import type {
   SentryWebpackPluginOptions,
   UserSentryOptions,
 } from './types';
+import { constructWebpackConfigFunction } from './webpack';
 
 /**
  * Add Sentry options to the config to be exported from the user's `next.config.js` file.
@@ -22,49 +21,44 @@ export function withSentryConfig(
   userSentryWebpackPluginOptions: Partial<SentryWebpackPluginOptions> = {},
   sentryOptions?: UserSentryOptions,
 ): NextConfigFunction | NextConfigObject {
-  return function (phase: string, defaults: { defaultConfig: NextConfigObject }): NextConfigObject {
-    const userNextConfigObject =
-      typeof exportedUserNextConfig === 'function' ? exportedUserNextConfig(phase, defaults) : exportedUserNextConfig;
-    // Inserts additional `sentry` options into the existing config, allows for backwards compatability
-    // in case nothing is passed into the optional `sentryOptions` argument
-    userNextConfigObject.sentry = { ...userNextConfigObject.sentry, ...sentryOptions };
-    return getFinalConfigObject(phase, userNextConfigObject, userSentryWebpackPluginOptions);
-  };
+  if (typeof exportedUserNextConfig === 'function') {
+    return function (this: unknown, ...webpackConfigFunctionArgs: unknown[]): NextConfigObject {
+      const userNextConfigObject: NextConfigObjectWithSentry = exportedUserNextConfig.apply(
+        this,
+        webpackConfigFunctionArgs,
+      );
+
+      const userSentryOptions = { ...userNextConfigObject.sentry, ...sentryOptions };
+      return getFinalConfigObject(userNextConfigObject, userSentryOptions, userSentryWebpackPluginOptions);
+    };
+  } else {
+    const userSentryOptions = { ...exportedUserNextConfig.sentry, ...sentryOptions };
+    return getFinalConfigObject(exportedUserNextConfig, userSentryOptions, userSentryWebpackPluginOptions);
+  }
 }
 
 // Modify the materialized object form of the user's next config by deleting the `sentry` property and wrapping the
 // `webpack` property
 function getFinalConfigObject(
-  phase: string,
   incomingUserNextConfigObject: NextConfigObjectWithSentry,
+  userSentryOptions: UserSentryOptions,
   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;
+  // Next 12.2.3+ warns about non-canonical properties on `userNextConfig`.
   delete incomingUserNextConfigObject.sentry;
-  // Remind TS that there's now no `sentry` property
-  const userNextConfigObject = incomingUserNextConfigObject as NextConfigObject;
 
   if (userSentryOptions?.tunnelRoute) {
-    setUpTunnelRewriteRules(userNextConfigObject, userSentryOptions.tunnelRoute);
+    setUpTunnelRewriteRules(incomingUserNextConfigObject, userSentryOptions.tunnelRoute);
   }
 
-  // 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 or in development mode.
-  if (phase === PHASE_PRODUCTION_BUILD || phase === PHASE_DEVELOPMENT_SERVER) {
-    // eslint-disable-next-line @typescript-eslint/no-var-requires
-    const { constructWebpackConfigFunction } = require('./webpack');
-    return {
-      ...userNextConfigObject,
-      webpack: constructWebpackConfigFunction(userNextConfigObject, userSentryWebpackPluginOptions, userSentryOptions),
-    };
-  }
-
-  // At runtime, we just return the user's config untouched.
-  return userNextConfigObject;
+  return {
+    ...incomingUserNextConfigObject,
+    webpack: constructWebpackConfigFunction(
+      incomingUserNextConfigObject,
+      userSentryWebpackPluginOptions,
+      userSentryOptions,
+    ),
+  };
 }
 
 /**

packages/nextjs/test/config/withSentryConfig.test.ts

@@ -59,46 +59,4 @@ 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 build phase is "phase-production-build"', () => {
-      jest.isolateModules(() => {
-        // In case this is still set from elsewhere, reset it
-        delete (global as any)._sentryWebpackModuleLoaded;
-
-        materializeFinalNextConfig(exportedNextConfig, undefined, 'phase-production-build');
-
-        expect((global as any)._sentryWebpackModuleLoaded).toBe(true);
-      });
-    });
-
-    it('imports from `webpack.ts` if build phase is "phase-development-server"', () => {
-      jest.isolateModules(() => {
-        // In case this is still set from elsewhere, reset it
-        delete (global as any)._sentryWebpackModuleLoaded;
-
-        materializeFinalNextConfig(exportedNextConfig, undefined, 'phase-production-build');
-
-        expect((global as any)._sentryWebpackModuleLoaded).toBe(true);
-      });
-    });
-
-    it('Doesn\'t import from `webpack.ts` if build phase is "phase-production-server"', () => {
-      jest.isolateModules(() => {
-        // In case this is still set from elsewhere, reset it
-        delete (global as any)._sentryWebpackModuleLoaded;
-
-        materializeFinalNextConfig(exportedNextConfig, undefined, 'phase-production-server');
-
-        expect((global as any)._sentryWebpackModuleLoaded).toBeUndefined();
-      });
-    });
-  });
 });