ref(nextjs): Use loader to set RewriteFrames helper value (#5445)

lobsterkatie · web-flow · commit 57c964a72706 · 2022-07-25T14:27:05.000-07:00
In order to work, in the nextjs SDK our server-side `RewriteFrames` integration needs to know the name of the directory into which the app is built. While that information is readily available at build time, it isn't at runtime, so we adjust the webpack config to inject a global variable with the correct value in before the the injected `Sentry.init()` call from `sentry.server.config.js` wherever we inject that `Sentry.init(). Currently, this is done by creating a temporary file with the relevant code and adding it to the relevant pages' `entry` value along with `sentry.server.config.js`. This works, but has its downsides[1], and is a fair amount of machinery just to add a single line of code. This injects the same line of code using a loader, which is really just a transformation function for the code from matching files. In this case, we're modifying `sentry.server.config.js` itself, so that by the time it's added to various pages' entry points, it already contains the relevant code. Since we don't know what the value will be ahead of time, there's a template, which the loader then populates before injecting the new code. _But how is that any less machinery?_, you might ask. _All of that, still for just one line of code?_ Honestly, it's not. But in the quest to improve parameterization of transactions names, we're going to be adding a loader in any case. Adding the `RewriteFrames` value thus provides the perfect proof of concept, precisely because it's so simple, letting us get the loader working separate from all of the other changes that work will entail. Notes: - I moved setting the default value for `distDir` from retrieval-of-the-value time to storage-of-the-value time, because using a loader necessarily stringifies everything, meaning `undefined` was turning into the string `'undefined'`, preventing the default from getting picked up. - There are a few scattered lines of unrelated changes, artifacts of the fact that I ended up unpacking a few values at the top of the new webpack function. [1] #4813
diff --git a/packages/nextjs/rollup.npm.config.js b/packages/nextjs/rollup.npm.config.js
@@ -1,12 +1,47 @@
 import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js';
 
-export default makeNPMConfigVariants(
-  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'],
-    // prevent this nextjs code from ending up in our built package (this doesn't happen automatially because the name
-    // doesn't match an SDK dependency)
-    packageSpecificConfig: { external: ['next/router'] },
-  }),
-);
+export default [
+  ...makeNPMConfigVariants(
+    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'],
+
+      // 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)
+      packageSpecificConfig: { external: ['next/router'] },
+    }),
+  ),
+  ...makeNPMConfigVariants(
+    makeBaseNPMConfig({
+      entrypoints: ['src/config/prefixLoaderTemplate.ts'],
+
+      packageSpecificConfig: {
+        output: {
+          // preserve the original file structure (i.e., so that everything is still relative to `src`)
+          entryFileNames: 'config/[name].js',
+
+          // this is going to be add-on code, so it doesn't need the trappings of a full module (and in fact actively
+          // shouldn't have them, lest they muck with the module to which we're adding it)
+          sourcemap: false,
+          esModule: false,
+        },
+      },
+    }),
+  ),
+  ...makeNPMConfigVariants(
+    makeBaseNPMConfig({
+      entrypoints: ['src/config/prefixLoader.ts'],
+
+      packageSpecificConfig: {
+        output: {
+          // make it so Rollup calms down about the fact that we're doing `export { loader as default }`
+          exports: 'default',
+
+          // preserve the original file structure (i.e., so that everything is still relative to `src`)
+          entryFileNames: 'config/[name].js',
+        },
+      },
+    }),
+  ),
+];
diff --git a/packages/nextjs/src/config/prefixLoader.ts b/packages/nextjs/src/config/prefixLoader.ts
@@ -0,0 +1,34 @@
+import * as fs from 'fs';
+import * as path from 'path';
+
+type LoaderOptions = {
+  distDir: string;
+};
+// TODO Use real webpack types
+type LoaderThis = {
+  // Webpack 4
+  query?: LoaderOptions;
+  // Webpack 5
+  getOptions?: () => LoaderOptions;
+  addDependency: (filepath: string) => void;
+};
+
+/**
+ * Inject templated code into the beginning of a module.
+ */
+function prefixLoader(this: LoaderThis, userCode: string): string {
+  // We know one or the other will be defined, depending on the version of webpack being used
+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  const { distDir } = this.getOptions ? this.getOptions() : this.query!;
+
+  const templatePath = path.resolve(__dirname, 'prefixLoaderTemplate.js');
+  this.addDependency(templatePath);
+
+  // Fill in the placeholder
+  let templateCode = fs.readFileSync(templatePath).toString();
+  templateCode = templateCode.replace('__DIST_DIR__', distDir);
+
+  return `${templateCode}\n${userCode}`;
+}
+
+export { prefixLoader as default };
diff --git a/packages/nextjs/src/config/prefixLoaderTemplate.ts b/packages/nextjs/src/config/prefixLoaderTemplate.ts
@@ -0,0 +1,5 @@
+// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
+(global as any).__rewriteFramesDistDir__ = '__DIST_DIR__';
+
+// We need this to make this file an ESM module, which TS requires when using `isolatedModules`.
+export {};
diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts
@@ -56,6 +56,15 @@ export type WebpackConfigObject = {
   resolve?: {
     alias?: { [key: string]: string | boolean };
   };
+  module?: {
+    rules: Array<{
+      test: string | RegExp;
+      use: Array<{
+        loader: string;
+        options: Record<string, unknown>;
+      }>;
+    }>;
+  };
 } & {
   // other webpack options
   [key: string]: unknown;
diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts
@@ -3,7 +3,6 @@ import { getSentryRelease } from '@sentry/node';
 import { dropUndefinedKeys, logger } from '@sentry/utils';
 import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin';
 import * as fs from 'fs';
-import * as os from 'os';
 import * as path from 'path';
 
 import {
@@ -42,6 +41,7 @@ export function constructWebpackConfigFunction(
   // we're building server or client, whether we're in dev, what version of webpack we're using, etc). Note that
   // `incomingConfig` and `buildContext` are referred to as `config` and `options` in the nextjs docs.
   const newWebpackFunction = (incomingConfig: WebpackConfigObject, buildContext: BuildContext): WebpackConfigObject => {
+    const { isServer, dev: isDev } = buildContext;
     let newConfig = { ...incomingConfig };
 
     // if user has custom webpack config (which always takes the form of a function), run it so we have actual values to
@@ -50,6 +50,29 @@ export function constructWebpackConfigFunction(
       newConfig = userNextConfig.webpack(newConfig, buildContext);
     }
 
+    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, 'prefixLoader.js'),
+                options: {
+                  distDir: userNextConfig.distDir || '.next',
+                },
+              },
+            ],
+          },
+        ],
+      };
+    }
+
     // Tell webpack to inject user config files (containing the two `Sentry.init()` calls) into the appropriate output
     // bundles. Store a separate reference to the original `entry` value to avoid an infinite loop. (If we don't do
     // this, we'll have a statement of the form `x.y = () => f(x.y)`, where one of the things `f` does is call `x.y`.
@@ -70,7 +93,7 @@ export function constructWebpackConfigFunction(
       // 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() &&
-      (buildContext.isServer
+      (isServer
         ? !userNextConfig.sentry?.disableServerWebpackPlugin
         : !userNextConfig.sentry?.disableClientWebpackPlugin);
 
@@ -80,14 +103,13 @@ export function constructWebpackConfigFunction(
 
       // Next doesn't let you change `devtool` in dev even if you want to, so don't bother trying - see
       // https://github.com/vercel/next.js/blob/master/errors/improper-devtool.md
-      if (!buildContext.dev) {
+      if (!isDev) {
         // `hidden-source-map` produces the same sourcemaps as `source-map`, but doesn't include the `sourceMappingURL`
         // comment at the bottom. For folks who aren't publicly hosting their sourcemaps, this is helpful because then
         // the browser won't look for them and throw errors into the console when it can't find them. Because this is a
         // front-end-only problem, and because `sentry-cli` handles sourcemaps more reliably with the comment than
         // without, the option to use `hidden-source-map` only applies to the client-side build.
-        newConfig.devtool =
-          userNextConfig.sentry?.hideSourceMaps && !buildContext.isServer ? 'hidden-source-map' : 'source-map';
+        newConfig.devtool = userNextConfig.sentry?.hideSourceMaps && !isServer ? 'hidden-source-map' : 'source-map';
       }
 
       newConfig.plugins = newConfig.plugins || [];
@@ -121,7 +143,7 @@ async function addSentryToEntryProperty(
   // we know is that it won't have gotten *simpler* in form, so we only need to worry about the object and function
   // options. See https://webpack.js.org/configuration/entry-context/#entry.
 
-  const { isServer, dir: projectDir, dev: isDev, config: userNextConfig } = buildContext;
+  const { isServer, dir: projectDir } = buildContext;
 
   const newEntryProperty =
     typeof currentEntryProperty === 'function' ? await currentEntryProperty() : { ...currentEntryProperty };
@@ -132,21 +154,6 @@ async function addSentryToEntryProperty(
   // we need to turn the filename into a path so webpack can find it
   const filesToInject = [`./${userConfigFile}`];
 
-  // 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). Doesn't work when using the dev server
-  // because it somehow tricks the file watcher into thinking that compilation itself is a file change, triggering an
-  // infinite recompiling loop. (This should be fine because we don't upload sourcemaps in dev in any case.)
-  if (isServer && !isDev) {
-    const rewriteFramesHelper = path.resolve(
-      fs.mkdtempSync(path.resolve(os.tmpdir(), 'sentry-')),
-      'rewriteFramesHelper.js',
-    );
-    fs.writeFileSync(rewriteFramesHelper, `global.__rewriteFramesDistDir__ = '${userNextConfig.distDir}';\n`);
-    // stick our helper file ahead of the user's config file so the value is in the global namespace *before*
-    // `Sentry.init()` is called
-    filesToInject.unshift(rewriteFramesHelper);
-  }
-
   // inject into all entry points which might contain user's code
   for (const entryPointName in newEntryProperty) {
     if (shouldAddSentryToEntryPoint(entryPointName, isServer)) {
diff --git a/packages/nextjs/src/index.server.ts b/packages/nextjs/src/index.server.ts
@@ -110,7 +110,7 @@ function sdkAlreadyInitialized(): boolean {
 
 function addServerIntegrations(options: NextjsOptions): void {
   // This value is injected at build time, based on the output directory specified in the build config
-  const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__ || '.next';
+  const distDirName = (global as GlobalWithDistDir).__rewriteFramesDistDir__;
   // nextjs always puts the build directory at the project root level, which is also where you run `next start` from, so
   // we can read in the project directory from the currently running process
   const distDirAbsPath = path.resolve(process.cwd(), distDirName);
diff --git a/packages/nextjs/test/config.test.ts b/packages/nextjs/test/config.test.ts
@@ -332,35 +332,26 @@ describe('webpack config', () => {
         incomingWebpackBuildContext: serverBuildContext,
       });
 
-      const tempDir = mkdtempSyncSpy.mock.results[0].value;
-      const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js');
-
       expect(finalWebpackConfig.entry).toEqual(
         expect.objectContaining({
           // original entrypoint value is a string
           // (was 'private-next-pages/_error.js')
-          'pages/_error': [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/_error.js'],
+          'pages/_error': [serverConfigFilePath, 'private-next-pages/_error.js'],
 
           // original entrypoint value is a string array
           // (was ['./node_modules/smellOVision/index.js', 'private-next-pages/_app.js'])
-          'pages/_app': [
-            rewriteFramesHelper,
-            serverConfigFilePath,
-            './node_modules/smellOVision/index.js',
-            'private-next-pages/_app.js',
-          ],
+          'pages/_app': [serverConfigFilePath, './node_modules/smellOVision/index.js', 'private-next-pages/_app.js'],
 
           // original entrypoint value is an object containing a string `import` value
           // (was { import: 'private-next-pages/api/simulator/dogStats/[name].js' })
           'pages/api/simulator/dogStats/[name]': {
-            import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'],
+            import: [serverConfigFilePath, 'private-next-pages/api/simulator/dogStats/[name].js'],
           },
 
           // original entrypoint value is an object containing a string array `import` value
           // (was { import: ['./node_modules/dogPoints/converter.js', 'private-next-pages/api/simulator/leaderboard.js'] })
           'pages/api/simulator/leaderboard': {
             import: [
-              rewriteFramesHelper,
               serverConfigFilePath,
               './node_modules/dogPoints/converter.js',
               'private-next-pages/api/simulator/leaderboard.js',
@@ -370,7 +361,7 @@ describe('webpack config', () => {
           // original entrypoint value is an object containg properties besides `import`
           // (was { import: 'private-next-pages/api/tricks/[trickName].js', dependOn: 'treats', })
           'pages/api/tricks/[trickName]': {
-            import: [rewriteFramesHelper, serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'],
+            import: [serverConfigFilePath, 'private-next-pages/api/tricks/[trickName].js'],
             dependOn: 'treats', // untouched
           },
         }),
@@ -478,53 +469,45 @@ describe('webpack config', () => {
         }),
       );
     });
+  });
 
-    it('does not inject `RewriteFrames` helper into client routes', async () => {
+  describe('webpack loaders', () => {
+    it('adds loader to server config', async () => {
+      const finalWebpackConfig = await materializeFinalWebpackConfig({
+        userNextConfig,
+        incomingWebpackConfig: serverWebpackConfig,
+        incomingWebpackBuildContext: serverBuildContext,
+      });
+
+      expect(finalWebpackConfig.module!.rules).toEqual(
+        expect.arrayContaining([
+          {
+            test: expect.any(RegExp),
+            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({}),
+              },
+            ],
+          },
+        ]),
+      );
+    });
+
+    it("doesn't add loader to client config", async () => {
       const finalWebpackConfig = await materializeFinalWebpackConfig({
         userNextConfig,
         incomingWebpackConfig: clientWebpackConfig,
         incomingWebpackBuildContext: clientBuildContext,
       });
 
-      expect(finalWebpackConfig.entry).toEqual(
-        expect.objectContaining({
-          // was 'next-client-pages-loader?page=%2F_app', and now has client config but not`RewriteFrames` helper injected
-          'pages/_app': [clientConfigFilePath, 'next-client-pages-loader?page=%2F_app'],
-        }),
-      );
+      expect(finalWebpackConfig.module).toBeUndefined();
     });
   });
 
   describe('`distDir` value in default server-side `RewriteFrames` integration', () => {
-    it.each([
-      ['no custom `distDir`', undefined, '.next'],
-      ['custom `distDir`', 'dist', 'dist'],
-    ])(
-      'creates file injecting `distDir` value into `global` - %s',
-      async (_name, customDistDir, expectedInjectedValue) => {
-        // Note: the fact that the file tested here gets injected correctly is covered in the 'webpack `entry` property
-        // config' tests above
-
-        const userNextConfigDistDir = {
-          ...userNextConfig,
-          ...(customDistDir && { distDir: customDistDir }),
-        };
-        await materializeFinalWebpackConfig({
-          userNextConfig: userNextConfigDistDir,
-          incomingWebpackConfig: serverWebpackConfig,
-          incomingWebpackBuildContext: getBuildContext('server', userNextConfigDistDir),
-        });
-
-        const tempDir = mkdtempSyncSpy.mock.results[0].value;
-        const rewriteFramesHelper = path.join(tempDir, 'rewriteFramesHelper.js');
-
-        expect(fs.existsSync(rewriteFramesHelper)).toBe(true);
-
-        const injectedCode = fs.readFileSync(rewriteFramesHelper).toString();
-        expect(injectedCode).toEqual(`global.__rewriteFramesDistDir__ = '${expectedInjectedValue}';\n`);
-      },
-    );
-
     describe('`RewriteFrames` ends up with correct `distDir` value', () => {
       // TODO: this, along with any number of other parts of the build process, should be tested with an integration
       // test which actually runs webpack and inspects the resulting bundles (and that integration test should test
