chore(nextjs): Cosmetic changes ahead of API route fix (#3784)

lobsterkatie · web-flow · commit 484aad577bf4 · 2021-07-08T11:21:53.000-07:00
Nothing substantive here - just a bunch of changes pulled out of an upcoming PR fixing API route monitoring in nextjs, to make that one easier to review.

Major themes:
  - Standardize naming
  - Split current and new values into separate variables
  - Remove old code
    - Check for entry point existence (moot given our minimum supported nextjs version)
    - Unnecessary stuff which came along when tracing code was copied from `instrumentServer.js`
diff --git a/packages/nextjs/src/config/types.ts b/packages/nextjs/src/config/types.ts
@@ -52,18 +52,14 @@ export type BuildContext = { dev: boolean; isServer: boolean; buildId: string };
 // For our purposes, the value for `entry` is either an object, or an async function which returns such an object
 export type WebpackEntryProperty = EntryPropertyObject | EntryPropertyFunction;
 
-// Each value in that object is either a string representing a single entry point, an array of such strings, or an
-// object containing either of those, along with other configuration options. In that third case, the entry point(s) are
-// listed under the key `import`.
 export type EntryPropertyObject = {
-  [key: string]:
-    | string
-    | Array<string>
-    // only in webpack 5
-    | EntryPointObject;
+  [key: string]: EntryPointValue;
 };
 
 export type EntryPropertyFunction = () => Promise<EntryPropertyObject>;
 
-// An object with options for a single entry point, potentially one of many in the webpack `entry` property
+// Each value in that object is either a string representing a single entry point, an array of such strings, or an
+// object containing either of those, along with other configuration options. In that third case, the entry point(s) are
+// listed under the key `import`.
+export type EntryPointValue = string | Array<string> | EntryPointObject;
 export type EntryPointObject = { import: string | Array<string> };
diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts
@@ -5,26 +5,25 @@ import * as SentryWebpackPlugin from '@sentry/webpack-plugin';
 import {
   BuildContext,
   EntryPointObject,
+  EntryPointValue,
   EntryPropertyObject,
   NextConfigObject,
   SentryWebpackPluginOptions,
   WebpackConfigFunction,
   WebpackConfigObject,
   WebpackEntryProperty,
 } from './types';
-import {
-  SENTRY_CLIENT_CONFIG_FILE,
-  SENTRY_SERVER_CONFIG_FILE,
-  SERVER_SDK_INIT_PATH,
-  storeServerConfigFileLocation,
-} from './utils';
+import { SERVER_SDK_INIT_PATH, storeServerConfigFileLocation } from './utils';
 
 export { SentryWebpackPlugin };
 
 // TODO: merge default SentryWebpackPlugin ignore with their SentryWebpackPlugin ignore or ignoreFile
 // TODO: merge default SentryWebpackPlugin include with their SentryWebpackPlugin include
 // TODO: drop merged keys from override check? `includeDefaults` option?
 
+const CLIENT_SDK_CONFIG_FILE = './sentry.client.config.js';
+const SERVER_SDK_CONFIG_FILE = './sentry.server.config.js';
+
 const defaultSentryWebpackPluginOptions = dropUndefinedKeys({
   url: process.env.SENTRY_URL,
   org: process.env.SENTRY_ORG,
@@ -53,9 +52,11 @@ export function constructWebpackConfigFunction(
   userNextConfig: NextConfigObject = {},
   userSentryWebpackPluginOptions: Partial<SentryWebpackPluginOptions> = {},
 ): WebpackConfigFunction {
-  const newWebpackFunction = (config: WebpackConfigObject, options: BuildContext): WebpackConfigObject => {
-    // clone to avoid mutability issues
-    let newConfig = { ...config };
+  // Will be called by nextjs and passed its default webpack configuration and context data about the build (whether
+  // 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 => {
+    let newConfig = { ...incomingConfig };
 
     // if we're building server code, store the webpack output path as an env variable, so we know where to look for the
     // webpack-processed version of `sentry.server.config.js` when we need it
@@ -66,7 +67,7 @@ export function constructWebpackConfigFunction(
     // 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, options);
+      newConfig = userNextConfig.webpack(newConfig, buildContext);
     }
 
     // Tell webpack to inject user config files (containing the two `Sentry.init()` calls) into the appropriate output
@@ -78,10 +79,10 @@ export function constructWebpackConfigFunction(
     // will call the callback which will call `f` which will call `x.y`... and on and on. Theoretically this could also
     // be fixed by using `bind`, but this is way simpler.)
     const origEntryProperty = newConfig.entry;
-    newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, options.isServer);
+    newConfig.entry = async () => addSentryToEntryProperty(origEntryProperty, buildContext);
 
     // Enable the Sentry plugin (which uploads source maps to Sentry when not in dev) by default
-    const enableWebpackPlugin = options.isServer
+    const enableWebpackPlugin = buildContext.isServer
       ? !userNextConfig.sentry?.disableServerWebpackPlugin
       : !userNextConfig.sentry?.disableClientWebpackPlugin;
 
@@ -92,7 +93,7 @@ export function constructWebpackConfigFunction(
 
       // Next doesn't let you change this is dev even if you want to - see
       // https://github.com/vercel/next.js/blob/master/errors/improper-devtool.md
-      if (!options.dev) {
+      if (!buildContext.dev) {
         newConfig.devtool = 'source-map';
       }
 
@@ -103,8 +104,8 @@ export function constructWebpackConfigFunction(
         // @ts-ignore Our types for the plugin are messed up somehow - TS wants this to be `SentryWebpackPlugin.default`,
         // but that's not actually a thing
         new SentryWebpackPlugin({
-          dryRun: options.dev,
-          release: getSentryRelease(options.buildId),
+          dryRun: buildContext.dev,
+          release: getSentryRelease(buildContext.buildId),
           ...defaultSentryWebpackPluginOptions,
           ...userSentryWebpackPluginOptions,
         }),
@@ -121,27 +122,23 @@ export function constructWebpackConfigFunction(
  * Modify the webpack `entry` property so that the code in `sentry.server.config.js` and `sentry.client.config.js` is
  * included in the the necessary bundles.
  *
- * @param origEntryProperty The value of the property before Sentry code has been injected
- * @param isServer A boolean provided by nextjs indicating whether we're handling the server bundles or the browser
- * bundles
+ * @param currentEntryProperty The value of the property before Sentry code has been injected
+ * @param buildContext Object passed by nextjs containing metadata about the build
  * @returns The value which the new `entry` property (which will be a function) will return (TODO: this should return
  * the function, rather than the function's return value)
  */
 async function addSentryToEntryProperty(
-  origEntryProperty: WebpackEntryProperty,
-  isServer: boolean,
+  currentEntryProperty: WebpackEntryProperty,
+  buildContext: BuildContext,
 ): Promise<EntryPropertyObject> {
   // The `entry` entry in a webpack config can be a string, array of strings, object, or function. By default, nextjs
   // sets it to an async function which returns the promise of an object of string arrays. Because we don't know whether
   // someone else has come along before us and changed that, we need to check a few things along the way. The one thing
   // 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.
 
-  let newEntryProperty = origEntryProperty;
-  if (typeof origEntryProperty === 'function') {
-    newEntryProperty = await origEntryProperty();
-  }
-  newEntryProperty = newEntryProperty as EntryPropertyObject;
+  const newEntryProperty =
+    typeof currentEntryProperty === 'function' ? await currentEntryProperty() : { ...currentEntryProperty };
 
   // Add a new element to the `entry` array, we force webpack to create a bundle out of the user's
   // `sentry.server.config.js` file and output it to `SERVER_INIT_LOCATION`. (See
@@ -152,14 +149,14 @@ async function addSentryToEntryProperty(
   // because that then forces the user into a particular TS config.)
 
   // On the server, create a separate bundle, as there's no one entry point depended on by all the others
-  if (isServer) {
+  if (buildContext.isServer) {
     // slice off the final `.js` since webpack is going to add it back in for us, and we don't want to end up with
     // `.js.js` as the extension
-    newEntryProperty[SERVER_SDK_INIT_PATH.slice(0, -3)] = SENTRY_SERVER_CONFIG_FILE;
+    newEntryProperty[SERVER_SDK_INIT_PATH.slice(0, -3)] = SERVER_SDK_CONFIG_FILE;
   }
   // On the client, it's sufficient to inject it into the `main` JS code, which is included in every browser page.
   else {
-    addFileToExistingEntryPoint(newEntryProperty, 'main', SENTRY_CLIENT_CONFIG_FILE);
+    addFileToExistingEntryPoint(newEntryProperty, 'main', CLIENT_SDK_CONFIG_FILE);
 
     // To work around a bug in nextjs, we need to ensure that the `main.js` entry is empty (otherwise it'll choose that
     // over `main` and we'll lose the change we just made). In case some other library has put something into it, copy
@@ -195,37 +192,44 @@ function addFileToExistingEntryPoint(
   filepath: string,
 ): void {
   // can be a string, array of strings, or object whose `import` property is one of those two
-  let injectedInto = entryProperty[entryPointName];
-
-  // Sometimes especially for older next.js versions it happens we don't have an entry point
-  if (!injectedInto) {
-    // eslint-disable-next-line no-console
-    console.error(`[Sentry] Can't inject ${filepath}, no entrypoint is defined.`);
-    return;
-  }
+  const currentEntryPoint = entryProperty[entryPointName];
+  let newEntryPoint: EntryPointValue;
 
   // We inject the user's client config file after the existing code so that the config file has access to
   // `publicRuntimeConfig`. See https://github.com/getsentry/sentry-javascript/issues/3485
-  if (typeof injectedInto === 'string') {
-    injectedInto = [injectedInto, filepath];
-  } else if (Array.isArray(injectedInto)) {
-    injectedInto = [...injectedInto, filepath];
-  } else {
-    let importVal: string | string[];
+  if (typeof currentEntryPoint === 'string') {
+    newEntryPoint = [currentEntryPoint, filepath];
+  } else if (Array.isArray(currentEntryPoint)) {
+    newEntryPoint = [...currentEntryPoint, filepath];
+  }
+  // descriptor object (webpack 5+)
+  else if (typeof currentEntryPoint === 'object' && 'import' in currentEntryPoint) {
+    const currentImportValue = currentEntryPoint.import;
+    let newImportValue: string | string[];
 
-    if (typeof injectedInto.import === 'string') {
-      importVal = [injectedInto.import, filepath];
+    if (typeof currentImportValue === 'string') {
+      newImportValue = [currentImportValue, filepath];
     } else {
-      importVal = [...injectedInto.import, filepath];
+      newImportValue = [...currentImportValue, filepath];
     }
 
-    injectedInto = {
-      ...injectedInto,
-      import: importVal,
+    newEntryPoint = {
+      ...currentEntryPoint,
+      import: newImportValue,
     };
+  } else {
+    // mimic the logger prefix in order to use `console.warn` (which will always be printed, regardless of SDK settings)
+    // eslint-disable-next-line no-console
+    console.error(
+      'Sentry Logger [Error]:',
+      `Could not inject SDK initialization code into entry point ${entryPointName}, as it is not a recognized format.\n`,
+      `Expected: string | Array<string> | { [key:string]: any, import: string | Array<string> }\n`,
+      `Got: ${currentEntryPoint}`,
+    );
+    return;
   }
 
-  entryProperty[entryPointName] = injectedInto;
+  entryProperty[entryPointName] = newEntryPoint;
 }
 
 /**
diff --git a/packages/nextjs/src/utils/handlers.ts b/packages/nextjs/src/utils/handlers.ts
@@ -20,7 +20,6 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => {
       if (currentScope) {
         currentScope.addEventProcessor(event => addRequestDataToEvent(event, req as NextRequest));
 
-        // We only want to record page and API requests
         if (hasTracingEnabled()) {
           // If there is a trace header set, extract the data from it (parentSpanId, traceId, and sampling decision)
           let traceparentData;
@@ -34,20 +33,18 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => {
           let reqPath = stripUrlQueryAndFragment(url);
           // Replace with placeholder
           if (req.query) {
+            // TODO get this from next if possible, to avoid accidentally replacing non-dynamic parts of the path if
+            // they match dynamic parts
             for (const [key, value] of Object.entries(req.query)) {
               reqPath = reqPath.replace(`${value}`, `[${key}]`);
             }
           }
-
-          // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction
-          // name; requests to API routes could be GET, POST, PUT, etc, so do include it there
-          const namePrefix = `${(req.method || 'GET').toUpperCase()} `;
+          const reqMethod = `${(req.method || 'GET').toUpperCase()} `;
 
           const transaction = startTransaction(
             {
-              name: `${namePrefix}${reqPath}`,
+              name: `${reqMethod}${reqPath}`,
               op: 'http.server',
-              metadata: { requestPath: reqPath },
               ...traceparentData,
             },
             // extra context passed to the `tracesSampler`
@@ -57,7 +54,7 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => {
         }
       }
 
-      return await handler(req, res); // Call Handler
+      return await handler(req, res); // Call original handler
     } catch (e) {
       withScope(scope => {
         scope.addEventProcessor(event => {
@@ -74,11 +71,6 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => {
       if (transaction) {
         transaction.setHttpStatus(res.statusCode);
 
-        // we'll collect this data in a more targeted way in the event processor we added above,
-        // `addRequestDataToEvent`
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-        delete transaction.metadata.requestPath;
-
         transaction.finish();
       }
       try {
