feat(nuxt): Make dynamic import() wrapping default (#13958)

s1gr1d · web-flow · commit 6bc37f09d7c3 · 2024-10-17T09:37:40.000+02:00
BREAKING CHANGE: The `--import` flag must not be added anymore. If it is still set, the server-side will be initialized twice and this leads to unexpected errors. --- First merge this: #13945 Part of this: #13943 This PR makes it the default to include a rollup plugin that wraps the server entry file with a dynamic import (`import()`). This is a replacement for the node `--import` CLI flag. If you still want to manually add the CLI flag you can use this option in the `nuxt.config.ts` file: ```js sentry: { dynamicImportForServerEntry: false, } ``` (option name is up for discussion)
diff --git a/dev-packages/e2e-tests/test-applications/nuxt-3/package.json b/dev-packages/e2e-tests/test-applications/nuxt-3/package.json
@@ -7,7 +7,7 @@
     "dev": "nuxt dev",
     "generate": "nuxt generate",
     "preview": "nuxt preview",
-    "start": "node --import ./.output/server/sentry.server.config.mjs .output/server/index.mjs",
+    "start": "node .output/server/index.mjs",
     "clean": "npx nuxi cleanup",
     "test": "playwright test",
     "test:build": "pnpm install && npx playwright install && pnpm build",
diff --git a/dev-packages/e2e-tests/test-applications/nuxt-4/package.json b/dev-packages/e2e-tests/test-applications/nuxt-4/package.json
@@ -7,7 +7,7 @@
     "dev": "nuxt dev",
     "generate": "nuxt generate",
     "preview": "nuxt preview",
-    "start": "node --import ./.output/server/sentry.server.config.mjs .output/server/index.mjs",
+    "start": "node .output/server/index.mjs",
     "clean": "npx nuxi cleanup",
     "test": "playwright test",
     "test:build": "pnpm install && npx playwright install && pnpm build",
diff --git a/packages/nuxt/src/common/types.ts b/packages/nuxt/src/common/types.ts
@@ -103,16 +103,19 @@ export type SentryNuxtModuleOptions = {
   debug?: boolean;
 
   /**
-   * Enabling basic server tracing can be used for environments where modifying the node option `--import` is not possible.
-   * However, enabling this option only supports limited tracing instrumentation. Only http traces will be collected (but no database-specific traces etc.).
+   * Wraps the server entry file with a dynamic `import()`. This will make it possible to preload Sentry and register
+   * necessary hooks before other code runs. (Node docs: https://nodejs.org/api/module.html#enabling)
    *
-   * If this option is `true`, the Sentry SDK will import the Sentry server config at the top of the server entry file to load the SDK on the server.
+   * If this option is `false`, the Sentry SDK won't wrap the server entry file with `import()`. Not wrapping the
+   * server entry file will disable Sentry on the server-side. When you set this option to `false`, make sure
+   * to add the Sentry server config with the node `--import` CLI flag to enable Sentry on the server-side.
    *
-   * **DO NOT** enable this option if you've already added the node option `--import` in your node start script. This would initialize Sentry twice on the server-side and leads to unexpected issues.
+   * **DO NOT** add the node CLI flag `--import` in your node start script, when `dynamicImportForServerEntry` is set to `true` (default).
+   * This would initialize Sentry twice on the server-side and this leads to unexpected issues.
    *
-   * @default false
+   * @default true
    */
-  experimental_basicServerTracing?: boolean;
+  dynamicImportForServerEntry?: boolean;
 
   /**
    * Options to be passed directly to the Sentry Rollup Plugin (`@sentry/rollup-plugin`) and Sentry Vite Plugin (`@sentry/vite-plugin`) that ship with the Sentry Nuxt SDK.
diff --git a/packages/nuxt/src/module.ts b/packages/nuxt/src/module.ts
@@ -2,7 +2,7 @@ import * as path from 'path';
 import { addPlugin, addPluginTemplate, addServerPlugin, createResolver, defineNuxtModule } from '@nuxt/kit';
 import { consoleSandbox } from '@sentry/utils';
 import type { SentryNuxtModuleOptions } from './common/types';
-import { addSentryTopImport, addServerConfigToBuild } from './vite/addServerConfig';
+import { addDynamicImportEntryFileWrapper, addServerConfigToBuild } from './vite/addServerConfig';
 import { setupSourceMaps } from './vite/sourceMaps';
 import { findDefaultSdkInitFile } from './vite/utils';
 
@@ -17,7 +17,12 @@ export default defineNuxtModule<ModuleOptions>({
     },
   },
   defaults: {},
-  setup(moduleOptions, nuxt) {
+  setup(moduleOptionsParam, nuxt) {
+    const moduleOptions = {
+      ...moduleOptionsParam,
+      dynamicImportForServerEntry: moduleOptionsParam.dynamicImportForServerEntry !== false, // default: true
+    };
+
     const moduleDirResolver = createResolver(import.meta.url);
     const buildDirResolver = createResolver(nuxt.options.buildDir);
 
@@ -48,15 +53,17 @@ export default defineNuxtModule<ModuleOptions>({
     const serverConfigFile = findDefaultSdkInitFile('server');
 
     if (serverConfigFile) {
-      // Inject the server-side Sentry config file with a side effect import
-      addPluginTemplate({
-        mode: 'server',
-        filename: 'sentry-server-config.mjs',
-        getContents: () =>
-          `import "${buildDirResolver.resolve(`/${serverConfigFile}`)}"\n` +
-          'import { defineNuxtPlugin } from "#imports"\n' +
-          'export default defineNuxtPlugin(() => {})',
-      });
+      if (moduleOptions.dynamicImportForServerEntry === false) {
+        // Inject the server-side Sentry config file with a side effect import
+        addPluginTemplate({
+          mode: 'server',
+          filename: 'sentry-server-config.mjs',
+          getContents: () =>
+            `import "${buildDirResolver.resolve(`/${serverConfigFile}`)}"\n` +
+            'import { defineNuxtPlugin } from "#imports"\n' +
+            'export default defineNuxtPlugin(() => {})',
+        });
+      }
 
       addServerPlugin(moduleDirResolver.resolve('./runtime/plugins/sentry.server'));
     }
@@ -67,11 +74,9 @@ export default defineNuxtModule<ModuleOptions>({
 
     nuxt.hooks.hook('nitro:init', nitro => {
       if (serverConfigFile && serverConfigFile.includes('.server.config')) {
-        addServerConfigToBuild(moduleOptions, nuxt, nitro, serverConfigFile);
+        if (moduleOptions.dynamicImportForServerEntry === false) {
+          addServerConfigToBuild(moduleOptions, nuxt, nitro, serverConfigFile);
 
-        if (moduleOptions.experimental_basicServerTracing) {
-          addSentryTopImport(moduleOptions, nitro);
-        } else {
           if (moduleOptions.debug) {
             const serverDirResolver = createResolver(nitro.options.output.serverDir);
             const serverConfigPath = serverDirResolver.resolve('sentry.server.config.mjs');
@@ -86,6 +91,17 @@ export default defineNuxtModule<ModuleOptions>({
               );
             });
           }
+        } else {
+          addDynamicImportEntryFileWrapper(nitro, serverConfigFile);
+
+          if (moduleOptions.debug) {
+            consoleSandbox(() => {
+              // eslint-disable-next-line no-console
+              console.log(
+                '[Sentry] Wrapping the server entry file with a dynamic `import()`, so Sentry can be preloaded before the server initializes.',
+              );
+            });
+          }
         }
       }
     });
diff --git a/packages/nuxt/src/vite/addServerConfig.ts b/packages/nuxt/src/vite/addServerConfig.ts
@@ -74,54 +74,6 @@ export function addServerConfigToBuild(
   });
 }
 
-/**
- *  Adds the Sentry server config import at the top of the server entry file to load the SDK on the server.
- *  This is necessary for environments where modifying the node option `--import` is not possible.
- *  However, only limited tracing instrumentation is supported when doing this.
- */
-export function addSentryTopImport(moduleOptions: SentryNuxtModuleOptions, nitro: Nitro): void {
-  nitro.hooks.hook('close', () => {
-    // other presets ('node-server' or 'vercel') have an index.mjs
-    const presetsWithServerFile = ['netlify'];
-    const entryFileName =
-      typeof nitro.options.rollupConfig?.output.entryFileNames === 'string'
-        ? nitro.options.rollupConfig?.output.entryFileNames
-        : presetsWithServerFile.includes(nitro.options.preset)
-          ? 'server.mjs'
-          : 'index.mjs';
-
-    const serverDirResolver = createResolver(nitro.options.output.serverDir);
-    const entryFilePath = serverDirResolver.resolve(entryFileName);
-
-    try {
-      fs.readFile(entryFilePath, 'utf8', (err, data) => {
-        const updatedContent = `import './${SERVER_CONFIG_FILENAME}.mjs';\n${data}`;
-
-        fs.writeFile(entryFilePath, updatedContent, 'utf8', () => {
-          if (moduleOptions.debug) {
-            consoleSandbox(() => {
-              // eslint-disable-next-line no-console
-              console.log(
-                `[Sentry] Successfully added the Sentry import to the server entry file "\`${entryFilePath}\`"`,
-              );
-            });
-          }
-        });
-      });
-    } catch (err) {
-      if (moduleOptions.debug) {
-        consoleSandbox(() => {
-          // eslint-disable-next-line no-console
-          console.warn(
-            `[Sentry] An error occurred when trying to add the Sentry import to the server entry file "\`${entryFilePath}\`":`,
-            err,
-          );
-        });
-      }
-    }
-  });
-}
-
 /**
  * This function modifies the Rollup configuration to include a plugin that wraps the entry file with a dynamic import (`import()`)
  * and adds the Sentry server config with the static `import` declaration.
@@ -187,11 +139,8 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug
           : resolution.id
               // Concatenates the query params to mark the file (also attaches names of re-exports - this is needed for serverless functions to re-export the handler)
               .concat(SENTRY_WRAPPED_ENTRY)
-              .concat(
-                exportedFunctions?.length
-                  ? SENTRY_FUNCTIONS_REEXPORT.concat(exportedFunctions.join(',')).concat(QUERY_END_INDICATOR)
-                  : '',
-              );
+              .concat(exportedFunctions?.length ? SENTRY_FUNCTIONS_REEXPORT.concat(exportedFunctions.join(',')) : '')
+              .concat(QUERY_END_INDICATOR);
       }
       return null;
     },
@@ -211,7 +160,7 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug
           // `import()` can be used for any code that should be run after the hooks are registered (https://nodejs.org/api/module.html#enabling)
           `import(${JSON.stringify(entryId)});\n` +
           // By importing "import-in-the-middle/hook.mjs", we can make sure this file wil be included, as not all node builders are including files imported with `module.register()`.
-          "import 'import-in-the-middle/hook.mjs'\n" +
+          "import 'import-in-the-middle/hook.mjs';\n" +
           `${reExportedFunctions}\n`
         );
       }
diff --git a/packages/nuxt/src/vite/utils.ts b/packages/nuxt/src/vite/utils.ts
@@ -57,7 +57,7 @@ export function extractFunctionReexportQueryParameters(query: string): string[]
   return match && match[1]
     ? match[1]
         .split(',')
-        .filter(param => param !== '' && param !== 'default')
+        .filter(param => param !== '')
         // Sanitize, as code could be injected with another rollup plugin
         .map((str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))
     : [];
@@ -72,10 +72,11 @@ export function constructFunctionReExport(pathWithQuery: string, entryId: string
   return functionNames.reduce(
     (functionsCode, currFunctionName) =>
       functionsCode.concat(
-        `export async function ${currFunctionName}(...args) {\n` +
+        'async function reExport(...args) {\n' +
           `  const res = await import(${JSON.stringify(entryId)});\n` +
           `  return res.${currFunctionName}.call(this, ...args);\n` +
-          '}\n',
+          '}\n' +
+          `export { reExport as ${currFunctionName} };\n`,
       ),
     '',
   );
diff --git a/packages/nuxt/test/vite/utils.test.ts b/packages/nuxt/test/vite/utils.test.ts
@@ -71,8 +71,11 @@ describe('findDefaultSdkInitFile', () => {
 describe('removeSentryQueryFromPath', () => {
   it('strips the Sentry query part from the path', () => {
     const url = `/example/path${SENTRY_WRAPPED_ENTRY}${SENTRY_FUNCTIONS_REEXPORT}foo,${QUERY_END_INDICATOR}`;
+    const url2 = `/example/path${SENTRY_WRAPPED_ENTRY}${QUERY_END_INDICATOR}`;
     const result = removeSentryQueryFromPath(url);
+    const result2 = removeSentryQueryFromPath(url2);
     expect(result).toBe('/example/path');
+    expect(result2).toBe('/example/path');
   });
 
   it('returns the same path if the specific query part is not present', () => {
@@ -85,7 +88,7 @@ describe('removeSentryQueryFromPath', () => {
 describe('extractFunctionReexportQueryParameters', () => {
   it.each([
     [`${SENTRY_FUNCTIONS_REEXPORT}foo,bar,${QUERY_END_INDICATOR}`, ['foo', 'bar']],
-    [`${SENTRY_FUNCTIONS_REEXPORT}foo,bar,default${QUERY_END_INDICATOR}`, ['foo', 'bar']],
+    [`${SENTRY_FUNCTIONS_REEXPORT}foo,bar,default${QUERY_END_INDICATOR}`, ['foo', 'bar', 'default']],
     [
       `${SENTRY_FUNCTIONS_REEXPORT}foo,a.b*c?d[e]f(g)h|i\\\\j(){hello},${QUERY_END_INDICATOR}`,
       ['foo', 'a\\.b\\*c\\?d\\[e\\]f\\(g\\)h\\|i\\\\\\\\j\\(\\)\\{hello\\}'],
@@ -108,18 +111,36 @@ describe('constructFunctionReExport', () => {
     const result2 = constructFunctionReExport(query2, entryId);
 
     const expected = `
-export async function foo(...args) {
+async function reExport(...args) {
   const res = await import("./module");
   return res.foo.call(this, ...args);
 }
-export async function bar(...args) {
+export { reExport as foo };
+async function reExport(...args) {
   const res = await import("./module");
   return res.bar.call(this, ...args);
-}`;
+}
+export { reExport as bar };
+`;
     expect(result.trim()).toBe(expected.trim());
     expect(result2.trim()).toBe(expected.trim());
   });
 
+  it('constructs re-export code for a "default" query parameters and entry ID', () => {
+    const query = `${SENTRY_FUNCTIONS_REEXPORT}default${QUERY_END_INDICATOR}}`;
+    const entryId = './index';
+    const result = constructFunctionReExport(query, entryId);
+
+    const expected = `
+async function reExport(...args) {
+  const res = await import("./index");
+  return res.default.call(this, ...args);
+}
+export { reExport as default };
+`;
+    expect(result.trim()).toBe(expected.trim());
+  });
+
   it('returns an empty string if the query string is empty', () => {
     const query = '';
     const entryId = './module';
