feat(nextjs): Parameterize prefix loader values (#6377)

lobsterkatie · web-flow · commit 1e78eb6399db · 2022-12-02T01:44:14.000-08:00
This changes the `prefixLoader` we use in the nextjs SDK to accept template name and replacement values as parameters, and pulls injection of that loader into a helper function. It also renames the current template from `prefixLoaderTemplate` to `serverRewriteFramesPrefixLoaderTemplate`, to better indicate its purpose and to distinguish it from future templates also using the prefix loader.
diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js
@@ -20,7 +20,7 @@ export default [
   ...makeNPMConfigVariants(
     makeBaseNPMConfig({
       entrypoints: [
-        'src/config/templates/prefixLoaderTemplate.ts',
+        'src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts',
         'src/config/templates/pageProxyLoaderTemplate.ts',
         'src/config/templates/apiProxyLoaderTemplate.ts',
       ],
diff --git a/packages/nextjs/src/config/loaders/prefixLoader.ts b/packages/nextjs/src/config/loaders/prefixLoader.ts
@@ -1,26 +1,38 @@
+import { escapeStringForRegex } from '@sentry/utils';
 import * as fs from 'fs';
 import * as path from 'path';
 
 import { LoaderThis } from './types';
 
 type LoaderOptions = {
-  distDir: string;
+  templatePrefix: string;
+  replacements: Array<[string, string]>;
 };
 
 /**
  * Inject templated code into the beginning of a module.
+ *
+ * Options:
+ *   - `templatePrefix`: The XXX in `XXXPrefixLoaderTemplate.ts`, to specify which template to use
+ *   - `replacements`: An array of tuples of the form `[<placeholder>, <replacementValue>]`, used for doing global
+ *        string replacement in the template. Note: The replacement is done sequentially, in the order in which the
+ *        replacement values are given. If any placeholder is a substring of any replacement value besides its own, make
+ *        sure to order the tuples in such a way as to avoid over-replacement.
  */
 export default function prefixLoader(this: LoaderThis<LoaderOptions>, userCode: string): string {
   // We know one or the other will be defined, depending on the version of webpack being used
-  const { distDir } = 'getOptions' in this ? this.getOptions() : this.query;
+  const { templatePrefix, replacements } = 'getOptions' in this ? this.getOptions() : this.query;
 
-  const templatePath = path.resolve(__dirname, '../templates/prefixLoaderTemplate.js');
+  const templatePath = path.resolve(__dirname, `../templates/${templatePrefix}PrefixLoaderTemplate.js`);
   // make sure the template is included when runing `webpack watch`
   this.addDependency(templatePath);
 
-  // Fill in the placeholder
+  // Fill in placeholders
   let templateCode = fs.readFileSync(templatePath).toString();
-  templateCode = templateCode.replace('__DIST_DIR__', distDir.replace(/\\/g, '\\\\'));
+  replacements.forEach(([placeholder, value]) => {
+    const placeholderRegex = new RegExp(escapeStringForRegex(placeholder), 'g');
+    templateCode = templateCode.replace(placeholderRegex, value);
+  });
 
   return `${templateCode}\n${userCode}`;
 }
diff --git a/packages/nextjs/src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts b/packages/nextjs/src/config/templates/serverRewriteFramesPrefixLoaderTemplate.ts
diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts
@@ -100,6 +100,9 @@ export type WebpackConfigObject = {
   [key: string]: unknown;
 };
 
+// A convenience type to save us from having to assert the existence of `module.rules` over and over
+export type WebpackConfigObjectWithModuleRules = WebpackConfigObject & Required<Pick<WebpackConfigObject, 'module'>>;
+
 // Information about the current build environment
 export type BuildContext = {
   dev: boolean;
diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts
@@ -23,6 +23,7 @@ import type {
   UserSentryOptions,
   WebpackConfigFunction,
   WebpackConfigObject,
+  WebpackConfigObjectWithModuleRules,
   WebpackEntryProperty,
   WebpackModuleRule,
   WebpackPluginInstance,
@@ -67,35 +68,21 @@ export function constructWebpackConfigFunction(
     buildContext: BuildContext,
   ): WebpackConfigObject {
     const { isServer, dev: isDev, dir: projectDir } = buildContext;
-    let newConfig = { ...incomingConfig };
+    let rawNewConfig = { ...incomingConfig };
 
     // if user has custom webpack config (which always takes the form of a function), run it so we have actual values to
     // work with
     if ('webpack' in userNextConfig && typeof userNextConfig.webpack === 'function') {
-      newConfig = userNextConfig.webpack(newConfig, buildContext);
+      rawNewConfig = userNextConfig.webpack(rawNewConfig, buildContext);
     }
 
+    // This mutates `rawNewConfig` in place, but also returns it in order to switch its type to one in which
+    // `newConfig.module.rules` is required, so we don't have to keep asserting its existence
+    const newConfig = setUpModuleRules(rawNewConfig);
+
     if (isServer) {
-      newConfig.module = {
-        ...newConfig.module,
-        rules: [
-          ...(newConfig.module?.rules || []),
-          {
-            test: /sentry\.server\.config\.(jsx?|tsx?)/,
-            use: [
-              {
-                // Support non-default output directories by making the output path (easy to get here at build-time)
-                // available to the server SDK's default `RewriteFrames` instance (which needs it at runtime), by
-                // injecting code to attach it to `global`.
-                loader: path.resolve(__dirname, 'loaders/prefixLoader.js'),
-                options: {
-                  distDir: userNextConfig.distDir || '.next',
-                },
-              },
-            ],
-          },
-        ],
-      };
+      // This loader will inject code setting global values for use by `RewriteFrames`
+      addRewriteFramesLoader(newConfig, 'server', userNextConfig);
 
       if (userSentryOptions.autoInstrumentServerFunctions !== false) {
         const pagesDir = newConfig.resolve?.alias?.['private-next-pages'] as string;
@@ -628,3 +615,59 @@ function handleSourcemapHidingOptionWarning(userSentryOptions: UserSentryOptions
   //     );
   //   }
 }
+
+/**
+ * Ensure that `newConfig.module.rules` exists. Modifies the given config in place but also returns it in order to
+ * change its type.
+ *
+ * @param newConfig A webpack config object which may or may not contain `module` and `module.rules`
+ * @returns The same object, with an empty `module.rules` array added if necessary
+ */
+function setUpModuleRules(newConfig: WebpackConfigObject): WebpackConfigObjectWithModuleRules {
+  newConfig.module = {
+    ...newConfig.module,
+    rules: [...(newConfig.module?.rules || [])],
+  };
+  // Surprising that we have to assert the type here, since we've demonstrably guaranteed the existence of
+  // `newConfig.module.rules` just above, but ¯\_(ツ)_/¯
+  return newConfig as WebpackConfigObjectWithModuleRules;
+}
+
+/**
+ * Support the `distDir` option by making its value (easy to get here at build-time) available to the server SDK's
+ * default `RewriteFrames` instance (which needs it at runtime), by injecting code to attach it to `global`.
+ *
+ * @param newConfig The webpack config object being constructed
+ * @param target Either 'server' or 'client'
+ * @param userNextConfig The user's nextjs config options
+ */
+function addRewriteFramesLoader(
+  newConfig: WebpackConfigObjectWithModuleRules,
+  target: 'server' | 'client',
+  userNextConfig: NextConfigObject,
+): void {
+  const replacements = {
+    server: [
+      [
+        '__DIST_DIR__',
+        // Make sure that if we have a windows path, the backslashes are interpreted as such (rather than as escape
+        // characters)
+        userNextConfig.distDir?.replace(/\\/g, '\\\\') || '.next',
+      ],
+    ],
+  };
+
+  newConfig.module.rules.push({
+    test: new RegExp(`sentry\\.${target}\\.config\\.(jsx?|tsx?)`),
+    use: [
+      {
+        loader: path.resolve(__dirname, 'loaders/prefixLoader.js'),
+        options: {
+          templatePrefix: `${target}RewriteFrames`,
+          // This weird cast will go away as soon as we add the client half of this function in
+          replacements: replacements[target as 'server'],
+        },
+      },
+    ],
+  });
+}
diff --git a/packages/nextjs/test/config/loaders.test.ts b/packages/nextjs/test/config/loaders.test.ts
@@ -10,39 +10,74 @@ import {
 } from './fixtures';
 import { materializeFinalWebpackConfig } from './testUtils';
 
+type MatcherResult = { pass: boolean; message: () => string };
+
+expect.extend({
+  stringEndingWith(received: string, expectedEnding: string): MatcherResult {
+    const failsTest = !received.endsWith(expectedEnding);
+    const generateErrorMessage = () =>
+      failsTest
+        ? // Regular error message for match failing
+          `expected string ending with '${expectedEnding}', but got '${received}'`
+        : // Error message for the match passing if someone has called it with `expect.not`
+          `expected string not ending with '${expectedEnding}', but got '${received}'`;
+
+    return {
+      pass: !failsTest,
+      message: generateErrorMessage,
+    };
+  },
+});
+
+declare global {
+  // eslint-disable-next-line @typescript-eslint/no-namespace
+  namespace jest {
+    interface Expect {
+      stringEndingWith: (expectedEnding: string) => MatcherResult;
+    }
+  }
+}
+
 describe('webpack loaders', () => {
-  it('adds loader to server config', async () => {
-    const finalWebpackConfig = await materializeFinalWebpackConfig({
-      exportedNextConfig,
-      incomingWebpackConfig: serverWebpackConfig,
-      incomingWebpackBuildContext: serverBuildContext,
+  describe('server loaders', () => {
+    it('adds server `RewriteFrames` loader to server config', async () => {
+      const finalWebpackConfig = await materializeFinalWebpackConfig({
+        exportedNextConfig,
+        incomingWebpackConfig: serverWebpackConfig,
+        incomingWebpackBuildContext: serverBuildContext,
+      });
+
+      expect(finalWebpackConfig.module.rules).toContainEqual({
+        test: /sentry\.server\.config\.(jsx?|tsx?)/,
+        use: [
+          {
+            loader: expect.stringEndingWith('prefixLoader.js'),
+            options: expect.objectContaining({ templatePrefix: 'serverRewriteFrames' }),
+          },
+        ],
+      });
     });
+  });
+
+  describe('client loaders', () => {
+    it("doesn't add `RewriteFrames` loader to client config", async () => {
+      const finalWebpackConfig = await materializeFinalWebpackConfig({
+        exportedNextConfig,
+        incomingWebpackConfig: clientWebpackConfig,
+        incomingWebpackBuildContext: clientBuildContext,
+      });
 
-    expect(finalWebpackConfig.module!.rules).toEqual(
-      expect.arrayContaining([
-        {
-          test: expect.any(RegExp),
+      expect(finalWebpackConfig.module.rules).not.toContainEqual(
+        expect.objectContaining({
           use: [
             {
-              loader: expect.any(String),
-              // Having no criteria for what the object contains is better than using `expect.any(Object)`, because that
-              // could be anything
-              options: expect.objectContaining({}),
+              loader: expect.stringEndingWith('prefixLoader.js'),
+              options: expect.objectContaining({ templatePrefix: expect.stringContaining('RewriteFrames') }),
             },
           ],
-        },
-      ]),
-    );
-  });
-
-  it("doesn't add loader to client config", async () => {
-    const finalWebpackConfig = await materializeFinalWebpackConfig({
-      exportedNextConfig,
-      incomingWebpackConfig: clientWebpackConfig,
-      incomingWebpackBuildContext: clientBuildContext,
+        }),
+      );
     });
-
-    expect(finalWebpackConfig.module).toBeUndefined();
   });
 });
 
diff --git a/packages/nextjs/test/config/testUtils.ts b/packages/nextjs/test/config/testUtils.ts
@@ -7,6 +7,7 @@ import {
   NextConfigObject,
   SentryWebpackPluginOptions,
   WebpackConfigObject,
+  WebpackConfigObjectWithModuleRules,
 } from '../../src/config/types';
 import { constructWebpackConfigFunction, SentryWebpackPlugin } from '../../src/config/webpack';
 import { withSentryConfig } from '../../src/config/withSentryConfig';
@@ -57,7 +58,7 @@ export async function materializeFinalWebpackConfig(options: {
   userSentryWebpackPluginConfig?: Partial<SentryWebpackPluginOptions>;
   incomingWebpackConfig: WebpackConfigObject;
   incomingWebpackBuildContext: BuildContext;
-}): Promise<WebpackConfigObject> {
+}): Promise<WebpackConfigObjectWithModuleRules> {
   const { exportedNextConfig, userSentryWebpackPluginConfig, incomingWebpackConfig, incomingWebpackBuildContext } =
     options;
 
@@ -83,7 +84,7 @@ export async function materializeFinalWebpackConfig(options: {
   const webpackEntryProperty = finalWebpackConfigValue.entry as EntryPropertyFunction;
   finalWebpackConfigValue.entry = await webpackEntryProperty();
 
-  return finalWebpackConfigValue;
+  return finalWebpackConfigValue as WebpackConfigObjectWithModuleRules;
 }
 
 // helper function to make sure we're checking the correct plugin's data
