fix(nextjs): Respect directives in value injection loader (#14083)

lforst · web-flow · commit e68865a50610 · 2024-10-28T14:03:07.000+01:00
This PR is in preparation for turbopack (#8105). In the future, `sentry.client.config.ts` will likely need to be configured with a `"use client"` directive so that turbopack knows it needs to be treated as a file on the client. Our value injection loader currently always prepends the `sentry.client.config.ts` file with statements, rendering any directives in the file useless and crashing turbopack when the file is attempted to be imported somewhere. This PR detects any comments and directives on top of a file to only inject values after.
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-13/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-13/sentry.client.config.ts
@@ -1,3 +1,5 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/sentry.client.config.ts
@@ -1,3 +1,5 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-15/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-15/sentry.client.config.ts
@@ -1,3 +1,5 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/sentry.client.config.ts
@@ -1,3 +1,5 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-t3/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-t3/sentry.client.config.ts
@@ -1,3 +1,5 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/app/layout.tsx b/dev-packages/e2e-tests/test-applications/nextjs-turbo/app/layout.tsx
@@ -1,7 +1,12 @@
+import { HackComponentToRunSideEffectsInSentryClientConfig } from '../sentry.client.config';
+
 export default function Layout({ children }: { children: React.ReactNode }) {
   return (
     <html lang="en">
-      <body>{children}</body>
+      <body>
+        <HackComponentToRunSideEffectsInSentryClientConfig />
+        {children}
+      </body>
     </html>
   );
 }
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/pages/[param]/pages-router-client-trace-propagation.tsx b/dev-packages/e2e-tests/test-applications/nextjs-turbo/pages/[param]/pages-router-client-trace-propagation.tsx
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/pages/_app.tsx b/dev-packages/e2e-tests/test-applications/nextjs-turbo/pages/_app.tsx
@@ -0,0 +1,6 @@
+import type { AppProps } from 'next/app';
+import '../sentry.client.config';
+
+export default function CustomApp({ Component, pageProps }: AppProps) {
+  return <Component {...pageProps} />;
+}
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/sentry.client.config.ts b/dev-packages/e2e-tests/test-applications/nextjs-turbo/sentry.client.config.ts
@@ -1,9 +1,17 @@
+'use client';
+
 import * as Sentry from '@sentry/nextjs';
 
-Sentry.init({
-  environment: 'qa', // dynamic sampling bias to keep transactions
-  dsn: process.env.NEXT_PUBLIC_E2E_TEST_DSN,
-  tunnel: `http://localhost:3031/`, // proxy server
-  tracesSampleRate: 1.0,
-  sendDefaultPii: true,
-});
+if (typeof window !== 'undefined') {
+  Sentry.init({
+    environment: 'qa', // dynamic sampling bias to keep transactions
+    dsn: process.env.NEXT_PUBLIC_E2E_TEST_DSN,
+    tunnel: `http://localhost:3031/`, // proxy server
+    tracesSampleRate: 1.0,
+    sendDefaultPii: true,
+  });
+}
+
+export function HackComponentToRunSideEffectsInSentryClientConfig() {
+  return null;
+}
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-turbo/tests/pages-router/client-trace-propagation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-turbo/tests/pages-router/client-trace-propagation.test.ts
@@ -1,25 +1,20 @@
 import { expect, test } from '@playwright/test';
 import { waitForTransaction } from '@sentry-internal/test-utils';
-import { extractTraceparentData } from '@sentry/utils';
 
 test('Should propagate traces from server to client in pages router', async ({ page }) => {
   const serverTransactionPromise = waitForTransaction('nextjs-turbo', async transactionEvent => {
-    return transactionEvent?.transaction === 'GET /[param]/client-trace-propagation';
+    return transactionEvent?.transaction === 'GET /[param]/pages-router-client-trace-propagation';
   });
 
-  await page.goto(`/123/client-trace-propagation`);
-
-  const sentryTraceLocator = await page.locator('meta[name="sentry-trace"]');
-  const sentryTraceValue = await sentryTraceLocator.getAttribute('content');
-  expect(sentryTraceValue).toMatch(/^[a-f0-9]{32}-[a-f0-9]{16}-[0-1]$/);
-
-  const baggageLocator = await page.locator('meta[name="baggage"]');
-  const baggageValue = await baggageLocator.getAttribute('content');
-  expect(baggageValue).toMatch(/sentry-public_key=/);
+  const pageloadTransactionPromise = waitForTransaction('nextjs-turbo', async transactionEvent => {
+    return transactionEvent?.transaction === '/[param]/pages-router-client-trace-propagation';
+  });
 
-  const traceparentData = extractTraceparentData(sentryTraceValue!);
+  await page.goto(`/123/pages-router-client-trace-propagation`);
 
   const serverTransaction = await serverTransactionPromise;
+  const pageloadTransaction = await pageloadTransactionPromise;
 
-  expect(serverTransaction.contexts?.trace?.trace_id).toBe(traceparentData?.traceId);
+  expect(serverTransaction.contexts?.trace?.trace_id).toBeDefined();
+  expect(pageloadTransaction.contexts?.trace?.trace_id).toBe(serverTransaction.contexts?.trace?.trace_id);
 });
diff --git a/packages/nextjs/src/config/loaders/valueInjectionLoader.ts b/packages/nextjs/src/config/loaders/valueInjectionLoader.ts
@@ -1,26 +1,43 @@
+// Rollup doesn't like if we put the directive regex as a literal (?). No idea why.
+/* eslint-disable @sentry-internal/sdk/no-regexp-constructor */
+
 import type { LoaderThis } from './types';
 
-type LoaderOptions = {
+export type ValueInjectionLoaderOptions = {
   values: Record<string, unknown>;
 };
 
+// We need to be careful not to inject anything before any `"use strict";`s or "use client"s or really any other directive.
+// As an additional complication directives may come after any number of comments.
+// This regex is shamelessly stolen from: https://github.com/getsentry/sentry-javascript-bundler-plugins/blob/7f984482c73e4284e8b12a08dfedf23b5a82f0af/packages/bundler-plugin-core/src/index.ts#L535-L539
+const SKIP_COMMENT_AND_DIRECTIVE_REGEX =
+  // Note: CodeQL complains that this regex potentially has n^2 runtime. This likely won't affect realistic files.
+  // biome-ignore lint/nursery/useRegexLiterals: No user input
+  new RegExp('^(?:\\s*|/\\*(?:.|\\r|\\n)*?\\*/|//.*[\\n\\r])*(?:"[^"]*";?|\'[^\']*\';?)?');
+
 /**
  * Set values on the global/window object at the start of a module.
  *
  * Options:
  *   - `values`: An object where the keys correspond to the keys of the global values to set and the values
  *        correspond to the values of the values on the global object. Values must be JSON serializable.
  */
-export default function valueInjectionLoader(this: LoaderThis<LoaderOptions>, userCode: string): string {
+export default function valueInjectionLoader(this: LoaderThis<ValueInjectionLoaderOptions>, userCode: string): string {
   // We know one or the other will be defined, depending on the version of webpack being used
   const { values } = 'getOptions' in this ? this.getOptions() : this.query;
 
   // We do not want to cache injected values across builds
   this.cacheable(false);
 
-  const injectedCode = Object.entries(values)
-    .map(([key, value]) => `globalThis["${key}"] = ${JSON.stringify(value)};`)
-    .join('\n');
+  // Not putting any newlines in the generated code will decrease the likelihood of sourcemaps breaking
+  const injectedCode =
+    // eslint-disable-next-line prefer-template
+    ';' +
+    Object.entries(values)
+      .map(([key, value]) => `globalThis["${key}"] = ${JSON.stringify(value)};`)
+      .join('');
 
-  return `${injectedCode}\n${userCode}`;
+  return userCode.replace(SKIP_COMMENT_AND_DIRECTIVE_REGEX, match => {
+    return match + injectedCode;
+  });
 }
diff --git a/packages/nextjs/test/config/__snapshots__/valueInjectionLoader.test.ts.snap b/packages/nextjs/test/config/__snapshots__/valueInjectionLoader.test.ts.snap
@@ -0,0 +1,83 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`valueInjectionLoader should correctly insert values for basic config 1`] = `
+"
+      ;globalThis[\\"foo\\"] = \\"bar\\";import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with a misplaced directive 1`] = `
+"
+      ;globalThis[\\"foo\\"] = \\"bar\\";console.log('This will render the directive useless');
+      \\"use client\\";
+
+
+
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive 1`] = `
+"
+      \\"use client\\";globalThis[\\"foo\\"] = \\"bar\\";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive and block comments 1`] = `
+"
+      /* test */
+      \\"use client\\";;globalThis[\\"foo\\"] = \\"bar\\";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive and inline comments 1`] = `
+"
+      // test
+      \\"use client\\";;globalThis[\\"foo\\"] = \\"bar\\";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive and multiline block comments 1`] = `
+"
+      /*
+        test
+      */
+      \\"use client\\";;globalThis[\\"foo\\"] = \\"bar\\";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive and multiline block comments and a bunch of whitespace 1`] = `
+"
+      /*
+        test
+      */
+
+
+
+
+      \\"use client\\";;globalThis[\\"foo\\"] = \\"bar\\";
+
+
+
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
+
+exports[`valueInjectionLoader should correctly insert values with directive and semicolon 1`] = `
+"
+      \\"use client\\";;globalThis[\\"foo\\"] = \\"bar\\";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    "
+`;
diff --git a/packages/nextjs/test/config/valueInjectionLoader.test.ts b/packages/nextjs/test/config/valueInjectionLoader.test.ts
@@ -0,0 +1,146 @@
+import type { LoaderThis } from '../../src/config/loaders/types';
+import type { ValueInjectionLoaderOptions } from '../../src/config/loaders/valueInjectionLoader';
+import valueInjectionLoader from '../../src/config/loaders/valueInjectionLoader';
+
+const defaultLoaderThis = {
+  addDependency: () => undefined,
+  async: () => undefined,
+  cacheable: () => undefined,
+  callback: () => undefined,
+};
+
+const loaderThis = {
+  ...defaultLoaderThis,
+  resourcePath: './client.config.ts',
+  getOptions() {
+    return {
+      values: {
+        foo: 'bar',
+      },
+    };
+  },
+} satisfies LoaderThis<ValueInjectionLoaderOptions>;
+
+describe('valueInjectionLoader', () => {
+  it('should correctly insert values for basic config', () => {
+    const userCode = `
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive', () => {
+    const userCode = `
+      "use client"
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive and semicolon', () => {
+    const userCode = `
+      "use client";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive and inline comments', () => {
+    const userCode = `
+      // test
+      "use client";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive and block comments', () => {
+    const userCode = `
+      /* test */
+      "use client";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive and multiline block comments', () => {
+    const userCode = `
+      /*
+        test
+      */
+      "use client";
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with directive and multiline block comments and a bunch of whitespace', () => {
+    const userCode = `
+      /*
+        test
+      */
+
+
+
+
+      "use client";
+
+
+
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+
+  it('should correctly insert values with a misplaced directive', () => {
+    const userCode = `
+      console.log('This will render the directive useless');
+      "use client";
+
+
+
+      import * as Sentry from '@sentry/nextjs';
+      Sentry.init();
+    `;
+
+    const result = valueInjectionLoader.call(loaderThis, userCode);
+
+    expect(result).toMatchSnapshot();
+    expect(result).toMatch(';globalThis["foo"] = "bar";');
+  });
+});

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+'use client';`
	`2`	`+`
`1`	`3`	`import * as Sentry from '@sentry/nextjs';`
`2`	`4`
`3`	`5`	`Sentry.init({`