review comments

Author: s1gr1d

packages/nuxt/src/vite/addServerConfig.ts

@@ -10,7 +10,7 @@ import {
   SENTRY_FUNCTIONS_REEXPORT,
   SENTRY_WRAPPED_ENTRY,
   constructFunctionReExport,
-  stripQueryPart,
+  removeSentryQueryFromPath,
 } from './utils';
 
 const SERVER_CONFIG_FILENAME = 'sentry.server.config';
@@ -147,11 +147,12 @@ export function addDynamicImportEntryFileWrapper(nitro: Nitro, serverConfigFile:
   );
 }
 
+/**
+ * A Rollup plugin which wraps the server entry with a dynamic `import()`. This makes it possible to initialize Sentry first
+ * by using a regular `import` and load the server after that.
+ * This also works with serverless `handler` functions, as it re-exports the `handler`.
+ */
 function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPluginOption {
-  const containsSuffix = (sourcePath: string): boolean => {
-    return sourcePath.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`) || sourcePath.includes(SENTRY_FUNCTIONS_REEXPORT);
-  };
-
   return {
     name: 'sentry-wrap-entry-with-dynamic-import',
     async resolveId(source, importer, options) {
@@ -160,27 +161,30 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug
       }
 
       if (source === 'import-in-the-middle/hook.mjs') {
+        // We are importing "import-in-the-middle" in the returned code of the `load()` function below
+        // By setting `moduleSideEffects` to `true`, the import is added to the bundle, although nothing is imported from it
+        // By importing "import-in-the-middle/hook.mjs", we can make sure this file is included, as not all node builders are including files imported with `module.register()`.
         return { id: source, moduleSideEffects: true, external: true };
       }
 
-      if (options.isEntry && !source.includes(SENTRY_WRAPPED_ENTRY)) {
+      if (options.isEntry && !source.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`)) {
         const resolution = await this.resolve(source, importer, options);
 
-        // If it cannot be resolved or is external, just return it
-        // so that Rollup can display an error
+        // If it cannot be resolved or is external, just return it so that Rollup can display an error
         if (!resolution || resolution?.external) return resolution;
 
         const moduleInfo = await this.load(resolution);
 
         moduleInfo.moduleSideEffects = true;
 
+        // The key `.` in `exportedBindings` refer to the exports within the file
         const exportedFunctions = moduleInfo.exportedBindings?.['.'];
 
-        // checks are needed to prevent multiple attachment of the suffix
-        return containsSuffix(source) || containsSuffix(resolution.id)
+        // The enclosing `if` already checks for the suffix in `source`, but a check in `resolution.id` is needed as well to prevent multiple attachment of the suffix
+        return resolution.id.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`)
           ? resolution.id
           : resolution.id
-              // concat the query params to mark the file (also attaches names of exports - this is needed for serverless functions to re-export the handler)
+              // 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
@@ -192,18 +196,21 @@ function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPlug
     },
     load(id: string) {
       if (id.includes(`.mjs${SENTRY_WRAPPED_ENTRY}`)) {
-        const entryId = stripQueryPart(id);
+        const entryId = removeSentryQueryFromPath(id);
 
+        // Mostly useful for serverless `handler` functions
         const reExportedFunctions = id.includes(SENTRY_FUNCTIONS_REEXPORT)
           ? constructFunctionReExport(id, entryId)
           : '';
 
         return (
-          // Import the Sentry server config
+          // Regular `import` of the Sentry config
           `import ${JSON.stringify(resolvedSentryConfigPath)};\n` +
-          // Dynamic import for the previous, actual entry point.
-          // import() can be used for any code that should be run after the hooks are registered (https://nodejs.org/api/module.html#enabling)
+          // Dynamic `import()` for the previous, actual entry point.
+          // `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" +
           `${reExportedFunctions}\n`
         );
       }

packages/nuxt/src/vite/utils.ts

@@ -30,18 +30,21 @@ export const SENTRY_FUNCTIONS_REEXPORT = '?sentry-query-functions-reexport=';
 export const QUERY_END_INDICATOR = 'SENTRY-QUERY-END';
 
 /**
- * Strips a specific query part from a URL.
+ * Strips the Sentry query part from a path.
+ * Example: example/path?sentry-query-wrapped-entry?sentry-query-functions-reexport=foo,SENTRY-QUERY-END -> /example/path
  *
  * Only exported for testing.
  */
-export function stripQueryPart(url: string): string {
+export function removeSentryQueryFromPath(url: string): string {
   // eslint-disable-next-line @sentry-internal/sdk/no-regexp-constructor
   const regex = new RegExp(`\\${SENTRY_WRAPPED_ENTRY}.*?\\${QUERY_END_INDICATOR}`);
   return url.replace(regex, '');
 }
 
 /**
- * Extracts and sanitizes function reexport query parameters from a query string.
+ * Extracts and sanitizes function re-export query parameters from a query string.
+ * If it is a default export, it is not considered for re-exporting. This function is mostly relevant for re-exporting
+ * serverless `handler` functions.
  *
  * Only exported for testing.
  */
@@ -55,7 +58,7 @@ export function extractFunctionReexportQueryParameters(query: string): string[]
     ? match[1]
         .split(',')
         .filter(param => param !== '' && param !== 'default')
-        // sanitize
+        // Sanitize, as code could be injected with another rollup plugin
         .map((str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))
     : [];
 }
@@ -69,8 +72,9 @@ export function constructFunctionReExport(pathWithQuery: string, entryId: string
   return functionNames.reduce(
     (functionsCode, currFunctionName) =>
       functionsCode.concat(
-        `export function ${currFunctionName}(...args) {\n` +
-          `  return import(${JSON.stringify(entryId)}).then((res) => res.${currFunctionName}(...args));\n` +
+        `export async function ${currFunctionName}(...args) {\n` +
+          `  const res = await import(${JSON.stringify(entryId)});\n` +
+          `  return res.${currFunctionName}.call(this, ...args);\n` +
           '}\n',
       ),
     '',

packages/nuxt/test/vite/utils.test.ts

@@ -7,7 +7,7 @@ import {
   constructFunctionReExport,
   extractFunctionReexportQueryParameters,
   findDefaultSdkInitFile,
-  stripQueryPart,
+  removeSentryQueryFromPath,
 } from '../../src/vite/utils';
 
 vi.mock('fs');
@@ -68,16 +68,16 @@ describe('findDefaultSdkInitFile', () => {
   });
 });
 
-describe('stripQueryPart', () => {
-  it('strips the specific query part from the URL', () => {
+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 result = stripQueryPart(url);
+    const result = removeSentryQueryFromPath(url);
     expect(result).toBe('/example/path');
   });
 
-  it('returns the same URL if the specific query part is not present', () => {
+  it('returns the same path if the specific query part is not present', () => {
     const url = '/example/path?other-query=param';
-    const result = stripQueryPart(url);
+    const result = removeSentryQueryFromPath(url);
     expect(result).toBe(url);
   });
 });
@@ -108,11 +108,13 @@ describe('constructFunctionReExport', () => {
     const result2 = constructFunctionReExport(query2, entryId);
 
     const expected = `
-export function foo(...args) {
-  return import("./module").then((res) => res.foo(...args));
+export async function foo(...args) {
+  const res = await import("./module");
+  return res.foo.call(this, ...args);
 }
-export function bar(...args) {
-  return import("./module").then((res) => res.bar(...args));
+export async function bar(...args) {
+  const res = await import("./module");
+  return res.bar.call(this, ...args);
 }`;
     expect(result.trim()).toBe(expected.trim());
     expect(result2.trim()).toBe(expected.trim());