fix(nextjs): Don't run webpack plugin on non-prod Vercel deployments (#5603)

lobsterkatie · web-flow · commit f411999657f9 · 2022-08-19T13:00:05.000-07:00
When someone adds our Vercel integration, it only adds sentry-cli-relevant environment variables to production deployments. This can cause non-production deployments to fail, because when the webpack loader tries to run, it doesn't find the information it needs. That said, there's a reason we only set the env variables for prod - we really shouldn't be creating releases or uploading sourcemaps for dev or preview deployments in any case. This adds an environment check for vercel deployments (using [the `VERCEL_ENV` env variable](https://vercel.com/docs/concepts/projects/environment-variables#system-environment-variables)), and only enables the webpack plugin if the environment is `production`. To give users a way to override this, setting `disableClientWebpackPlugin` or `disableServerWebpackPlugin` to `false` now takes precedence over other checks, rather than being a no-op. Finally, given that the number of enablement checks is likely to grow, this pulls them into a separate function.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,10 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+This release adds an environment check in `@sentry/nextjs` for Vercel deployments (using the `VERCEL_ENV` env variable), and only enables `SentryWebpackPlugin` if the environment is `production`. To override this, [setting `disableClientWebpackPlugin` or `disableServerWebpackPlugin` to `false`](https://docs.sentry.io/platforms/javascript/guides/nextjs/manual-setup/#disable-sentrywebpackplugin) now takes precedence over other checks, rather than being a no-op. Note: Overriding this is not recommended! It can increase build time and clog Release Health data in Sentry with inaccurate noise.
+
+- fix(nextjs): Don't run webpack plugin on non-prod Vercel deployments (#5603)
+
 ## 7.11.1
 
 - fix(remix): Store transaction on express req (#5595)
diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts
@@ -35,6 +35,8 @@ export type NextConfigObject = {
 };
 
 export type UserSentryOptions = {
+  // Override the SDK's default decision about whether or not to enable to the webpack plugin. Note that `false` forces
+  // the plugin to be enabled, even in situations where it's not recommended.
   disableServerWebpackPlugin?: boolean;
   disableClientWebpackPlugin?: boolean;
   hideSourceMaps?: boolean;
diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts
@@ -134,17 +134,7 @@ export function constructWebpackConfigFunction(
     newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext);
 
     // Enable the Sentry plugin (which uploads source maps to Sentry when not in dev) by default
-    const enableWebpackPlugin =
-      // TODO: this is a hack to fix https://github.com/getsentry/sentry-cli/issues/1085, which is caused by
-      // https://github.com/getsentry/sentry-cli/issues/915. Once the latter is addressed, this existence check can come
-      // out. (The check is necessary because currently, `@sentry/cli` uses a post-install script to download an
-      // architecture-specific version of the `sentry-cli` binary. If `yarn install`, `npm install`, or `npm ci` are run
-      // with the `--ignore-scripts` option, this will be blocked and the missing binary will cause an error when users
-      // try to build their apps.)
-      ensureCLIBinaryExists() &&
-      (isServer ? !userSentryOptions.disableServerWebpackPlugin : !userSentryOptions.disableClientWebpackPlugin);
-
-    if (enableWebpackPlugin) {
+    if (shouldEnableWebpackPlugin(buildContext, userSentryOptions)) {
       // TODO Handle possibility that user is using `SourceMapDevToolPlugin` (see
       // https://webpack.js.org/plugins/source-map-dev-tool-plugin/)
 
@@ -460,3 +450,47 @@ function ensureCLIBinaryExists(): boolean {
   }
   return false;
 }
+
+/** Check various conditions to decide if we should run the plugin */
+function shouldEnableWebpackPlugin(buildContext: BuildContext, userSentryOptions: UserSentryOptions): boolean {
+  const { isServer, dev: isDev } = buildContext;
+  const { disableServerWebpackPlugin, disableClientWebpackPlugin } = userSentryOptions;
+
+  /** Non-negotiable */
+
+  // TODO: this is a hack to fix https://github.com/getsentry/sentry-cli/issues/1085, which is caused by
+  // https://github.com/getsentry/sentry-cli/issues/915. Once the latter is addressed, this existence check can come
+  // out. (The check is necessary because currently, `@sentry/cli` uses a post-install script to download an
+  // architecture-specific version of the `sentry-cli` binary. If `yarn install`, `npm install`, or `npm ci` are run
+  // with the `--ignore-scripts` option, this will be blocked and the missing binary will cause an error when users
+  // try to build their apps.)
+  if (!ensureCLIBinaryExists()) {
+    return false;
+  }
+
+  /** User override */
+
+  if (isServer && disableServerWebpackPlugin !== undefined) {
+    return !disableServerWebpackPlugin;
+  } else if (!isServer && disableClientWebpackPlugin !== undefined) {
+    return !disableClientWebpackPlugin;
+  }
+
+  /** Situations where the default is to disable the plugin */
+
+  // TODO: Are there analogs to Vercel's preveiw and dev modes on other deployment platforms?
+
+  if (isDev || process.env.NODE_ENV === 'development') {
+    // TODO (v8): Right now in dev we set the plugin to dryrun mode, and our boilerplate includes setting the plugin to
+    // `silent`, so for the vast majority of users, it's as if the plugin doesn't run at all in dev. Making that
+    // official is technically a breaking change, though, so we probably should wait until v8.
+    // return false
+  }
+
+  if (process.env.VERCEL_ENV === 'preview' || process.env.VERCEL_ENV === 'development') {
+    return false;
+  }
+
+  // We've passed all of the tests!
+  return true;
+}
diff --git a/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts b/packages/nextjs/test/config/webpack/constructWebpackConfig.test.ts
@@ -12,7 +12,7 @@ import {
   serverWebpackConfig,
   userNextConfig,
 } from '../fixtures';
-import { materializeFinalWebpackConfig } from '../testUtils';
+import { materializeFinalNextConfig, materializeFinalWebpackConfig } from '../testUtils';
 
 describe('constructWebpackConfigFunction()', () => {
   it('includes expected properties', async () => {
@@ -47,6 +47,17 @@ describe('constructWebpackConfigFunction()', () => {
     expect(finalWebpackConfig).toEqual(expect.objectContaining(materializedUserWebpackConfig));
   });
 
+  it("doesn't set devtool if webpack plugin is disabled", () => {
+    const finalNextConfig = materializeFinalNextConfig({
+      ...exportedNextConfig,
+      webpack: () => ({ devtool: 'something-besides-source-map' } as any),
+      sentry: { disableServerWebpackPlugin: true },
+    });
+    const finalWebpackConfig = finalNextConfig.webpack?.(serverWebpackConfig, serverBuildContext);
+
+    expect(finalWebpackConfig?.devtool).not.toEqual('source-map');
+  });
+
   it('allows for the use of `hidden-source-map` as `devtool` value for client-side builds', async () => {
     const exportedNextConfigHiddenSourceMaps = {
       ...exportedNextConfig,
diff --git a/packages/nextjs/test/config/webpack/sentryWebpackPlugin.test.ts b/packages/nextjs/test/config/webpack/sentryWebpackPlugin.test.ts
@@ -2,7 +2,7 @@ import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
 
-import { BuildContext } from '../../../src/config/types';
+import { BuildContext, ExportedNextConfig } from '../../../src/config/types';
 import { getUserConfigFile, getWebpackPluginOptions, SentryWebpackPlugin } from '../../../src/config/webpack';
 import {
   clientBuildContext,
@@ -14,7 +14,7 @@ import {
   userSentryWebpackPluginConfig,
 } from '../fixtures';
 import { exitsSync, mkdtempSyncSpy, mockExistsSync, realExistsSync } from '../mocks';
-import { findWebpackPlugin, materializeFinalNextConfig, materializeFinalWebpackConfig } from '../testUtils';
+import { findWebpackPlugin, materializeFinalWebpackConfig } from '../testUtils';
 
 describe('Sentry webpack plugin config', () => {
   it('includes expected properties', async () => {
@@ -283,44 +283,111 @@ describe('Sentry webpack plugin config', () => {
     });
   });
 
-  describe('disabling SentryWebpackPlugin', () => {
-    it('allows SentryWebpackPlugin to be turned off for client code (independent of server code)', () => {
-      const clientFinalNextConfig = materializeFinalNextConfig({
-        ...exportedNextConfig,
-        sentry: { disableClientWebpackPlugin: true },
-      });
-      const clientFinalWebpackConfig = clientFinalNextConfig.webpack?.(clientWebpackConfig, clientBuildContext);
+  describe('SentryWebpackPlugin enablement', () => {
+    let processEnvBackup: typeof process.env;
 
-      const serverFinalNextConfig = materializeFinalNextConfig(exportedNextConfig, userSentryWebpackPluginConfig);
-      const serverFinalWebpackConfig = serverFinalNextConfig.webpack?.(serverWebpackConfig, serverBuildContext);
-
-      expect(clientFinalWebpackConfig?.plugins).not.toEqual(expect.arrayContaining([expect.any(SentryWebpackPlugin)]));
-      expect(serverFinalWebpackConfig?.plugins).toEqual(expect.arrayContaining([expect.any(SentryWebpackPlugin)]));
+    beforeEach(() => {
+      processEnvBackup = { ...process.env };
     });
-    it('allows SentryWebpackPlugin to be turned off for server code (independent of client code)', () => {
-      const serverFinalNextConfig = materializeFinalNextConfig({
-        ...exportedNextConfig,
-        sentry: { disableServerWebpackPlugin: true },
-      });
-      const serverFinalWebpackConfig = serverFinalNextConfig.webpack?.(serverWebpackConfig, serverBuildContext);
 
-      const clientFinalNextConfig = materializeFinalNextConfig(exportedNextConfig, userSentryWebpackPluginConfig);
-      const clientFinalWebpackConfig = clientFinalNextConfig.webpack?.(clientWebpackConfig, clientBuildContext);
-
-      expect(serverFinalWebpackConfig?.plugins).not.toEqual(expect.arrayContaining([expect.any(SentryWebpackPlugin)]));
-      expect(clientFinalWebpackConfig?.plugins).toEqual(expect.arrayContaining([expect.any(SentryWebpackPlugin)]));
+    afterEach(() => {
+      process.env = processEnvBackup;
     });
 
-    it("doesn't set devtool if webpack plugin is disabled", () => {
-      const finalNextConfig = materializeFinalNextConfig({
-        ...exportedNextConfig,
-        webpack: () => ({ devtool: 'something-besides-source-map' } as any),
-        sentry: { disableServerWebpackPlugin: true },
-      });
-      const finalWebpackConfig = finalNextConfig.webpack?.(serverWebpackConfig, serverBuildContext);
-
-      expect(finalWebpackConfig?.devtool).not.toEqual('source-map');
-    });
+    it.each([
+      // [testName, exportedNextConfig, extraEnvValues, shouldFindServerPlugin, shouldFindClientPlugin]
+      [
+        'obeys `disableClientWebpackPlugin = true`',
+        {
+          ...exportedNextConfig,
+          sentry: { disableClientWebpackPlugin: true },
+        },
+        {},
+        true,
+        false,
+      ],
+
+      [
+        'obeys `disableServerWebpackPlugin = true`',
+        {
+          ...exportedNextConfig,
+          sentry: { disableServerWebpackPlugin: true },
+        },
+        {},
+        false,
+        true,
+      ],
+      [
+        'disables the plugin in Vercel `preview` environment',
+        exportedNextConfig,
+        { VERCEL_ENV: 'preview' },
+        false,
+        false,
+      ],
+      [
+        'disables the plugin in Vercel `development` environment',
+        exportedNextConfig,
+        { VERCEL_ENV: 'development' },
+        false,
+        false,
+      ],
+      [
+        'allows `disableClientWebpackPlugin = false` to override env vars`',
+        {
+          ...exportedNextConfig,
+          sentry: { disableClientWebpackPlugin: false },
+        },
+        { VERCEL_ENV: 'preview' },
+        false,
+        true,
+      ],
+      [
+        'allows `disableServerWebpackPlugin = false` to override env vars`',
+        {
+          ...exportedNextConfig,
+          sentry: { disableServerWebpackPlugin: false },
+        },
+        { VERCEL_ENV: 'preview' },
+        true,
+        false,
+      ],
+    ])(
+      '%s',
+      async (
+        _testName: string,
+        exportedNextConfig: ExportedNextConfig,
+        extraEnvValues: Record<string, string>,
+        shouldFindServerPlugin: boolean,
+        shouldFindClientPlugin: boolean,
+      ) => {
+        process.env = { ...process.env, ...extraEnvValues };
+
+        // We create a copy of the next config for each `materializeFinalWebpackConfig` call because the `sentry`
+        // property gets deleted along the way, and its value matters for some of our test cases
+        const serverFinalWebpackConfig = await materializeFinalWebpackConfig({
+          exportedNextConfig: { ...exportedNextConfig },
+          userSentryWebpackPluginConfig,
+          incomingWebpackConfig: serverWebpackConfig,
+          incomingWebpackBuildContext: serverBuildContext,
+        });
+
+        const clientFinalWebpackConfig = await materializeFinalWebpackConfig({
+          exportedNextConfig: { ...exportedNextConfig },
+          userSentryWebpackPluginConfig,
+          incomingWebpackConfig: clientWebpackConfig,
+          incomingWebpackBuildContext: clientBuildContext,
+        });
+
+        const genericSentryWebpackPluginInstance = expect.any(SentryWebpackPlugin);
+
+        expect(findWebpackPlugin(serverFinalWebpackConfig, 'SentryCliPlugin')).toEqual(
+          shouldFindServerPlugin ? genericSentryWebpackPluginInstance : undefined,
+        );
+        expect(findWebpackPlugin(clientFinalWebpackConfig, 'SentryCliPlugin')).toEqual(
+          shouldFindClientPlugin ? genericSentryWebpackPluginInstance : undefined,
+        );
+      },
+    );
   });
 
   describe('getUserConfigFile', () => {
